Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an error that may occur shortly after worker replacement #49

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

zten
Copy link
Contributor

@zten zten commented Dec 18, 2023

If a worker is brand new, sometimes in start_inference() the VRAM model eviction might compare against
HordeProcessInfo.last_control_flag and discover that there is no such property.

... and that's how I discovered the field declarations at the top of classes are just documentation, and you still need to assign something in the constructor.

zten and others added 2 commits December 17, 2023 19:49
If a worker is brand new, sometimes in start_inference()
the VRAM model eviction might compare against
HordeProcessInfo.last_control_flag and discover that
there is no such property.

... and that's how I discovered the field declarations
at the top of classes are just documentation, and you
still need to assign something in the constructor.
Avoids uses the dataclass/pydantic way of relying on class members to set defaults for `HordeProcessInfo` (which is not a dataclass or pydantic BaseModel).
Copy link
Member

@tazlin tazlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the other default values for HordeProcessInfo to the __init__(...) for consistency. Default member values are bound to the class type and while I tend to feel that aliasing the class member with a value of None (or other defaults) with instance versions of those members is not a big deal, having the default definition in __init__(...) does better represent that these are instance variables.

@tazlin tazlin merged commit 2495687 into Haidra-Org:main Dec 18, 2023
1 check passed
@zten
Copy link
Contributor Author

zten commented Dec 18, 2023

Oh, I see the mistake I made. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants