-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update the State class #162
Merged
jeremyfowers
merged 16 commits into
refresh
from
152-refresh-eliminate-the-state-dataclass-and-stateyaml-file
Jun 19, 2024
Merged
Update the State class #162
jeremyfowers
merged 16 commits into
refresh
from
152-refresh-eliminate-the-state-dataclass-and-stateyaml-file
Jun 19, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeremyfowers
commented
Jun 18, 2024
jeremyfowers
commented
Jun 18, 2024
jeremyfowers
commented
Jun 18, 2024
jeremyfowers
commented
Jun 18, 2024
jeremyfowers
commented
Jun 18, 2024
jeremyfowers
commented
Jun 18, 2024
ramkrishna2910
approved these changes
Jun 18, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some minor comment!
This PR greatly simplifies the code :)
danielholanda
approved these changes
Jun 18, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Co-authored-by: Ramakrishnan Sivakumar <[email protected]> Signed-off-by: Jeremy Fowers <[email protected]>
Co-authored-by: Ramakrishnan Sivakumar <[email protected]> Signed-off-by: Jeremy Fowers <[email protected]>
…-file' of github.com:onnx/turnkeyml into 152-refresh-eliminate-the-state-dataclass-and-stateyaml-file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #152 by making the State class a lot more flexible.
Design considerations:
Dict
would not.state.yaml
entirely because we still need it for assessing cache hits/misses.Big changes:
State
has been flattened and generalized.State
by assigning them as attributes, iestate.new_member = value
.Config
class (e.g.,build_name
is now instate.build_name
instead ofstate.config.build_name
.State
is no longer adataclass
State
is self-initializing and no longer relies on external functions inignition
to set its defaults.Smaller changes:
Most
Enum
s have been converted to regular classes..value
statements have been removed.State.results
is no longer a list. It simply contains whatever result aStage
wants to produce. Code should stop referencingresults[0]
or settingresults = [output]
.State
has been moved tocommon.filesystem
to avoid circular imports.filesystem
tofs
in all modules (some didimport ... as filesystem
, now they are allimport ... as fs
)import build
toimport filesystem as fs