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

Update the State class #162

Conversation

jeremyfowers
Copy link
Collaborator

@jeremyfowers jeremyfowers commented Jun 17, 2024

Closes #152 by making the State class a lot more flexible.

Design considerations:

  1. The main intention here was to eliminate the breaking changes and crashes that could result from making simple changes to the State class.
  2. It turned out to be nice to keep a State class after all, since it can provide self-initialization and helper methods that a Dict would not.
  3. It turns out we cant get rid of state.yaml entirely because we still need it for assessing cache hits/misses.

Big changes:

  • State has been flattened and generalized.
    • There are far fewer hardcoded members.
    • Developers can add new members to State by assigning them as attributes, ie state.new_member = value.
    • There is no more Config class (e.g., build_name is now in state.build_name instead of state.config.build_name.
    • State is no longer a dataclass
  • State is self-initializing and no longer relies on external functions in ignition to set its defaults.

Smaller changes:

  • Most Enums have been converted to regular classes.

    • This allows us to safely save and load their values from YAML without worrying about converting to and from the Enum class.
    • Lots of .value statements have been removed.
  • State.results is no longer a list. It simply contains whatever result a Stage wants to produce. Code should stop referencing results[0] or setting results = [output].

  • State has been moved to common.filesystem to avoid circular imports.

    • Unified import of filesystem to fs in all modules (some did import ... as filesystem, now they are all import ... as fs)
    • Results in lots of changed of import build to import filesystem as fs

@jeremyfowers jeremyfowers self-assigned this Jun 17, 2024
@jeremyfowers jeremyfowers changed the title 152 refresh eliminate the state dataclass and stateyaml file Update the State class Jun 18, 2024
setup.py Show resolved Hide resolved
@jeremyfowers jeremyfowers marked this pull request as ready for review June 18, 2024 15:19
Copy link
Collaborator

@ramkrishna2910 ramkrishna2910 left a 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 :)

src/turnkeyml/build/ignition.py Show resolved Hide resolved
src/turnkeyml/cli/report.py Outdated Show resolved Hide resolved
src/turnkeyml/common/filesystem.py Outdated Show resolved Hide resolved
src/turnkeyml/common/filesystem.py Outdated Show resolved Hide resolved
src/turnkeyml/common/filesystem.py Show resolved Hide resolved
Copy link
Collaborator

@danielholanda danielholanda left a comment

Choose a reason for hiding this comment

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

Looking good!

setup.py Show resolved Hide resolved
src/turnkeyml/build/export.py Show resolved Hide resolved
src/turnkeyml/common/filesystem.py Show resolved Hide resolved
test/build_model.py Show resolved Hide resolved
jeremyfowers and others added 4 commits June 18, 2024 20:02
Co-authored-by: Ramakrishnan Sivakumar <ramkrishna2910@gmail.com>
Signed-off-by: Jeremy Fowers <80718789+jeremyfowers@users.noreply.github.com>
Co-authored-by: Ramakrishnan Sivakumar <ramkrishna2910@gmail.com>
Signed-off-by: Jeremy Fowers <80718789+jeremyfowers@users.noreply.github.com>
…-file' of github.com:onnx/turnkeyml into 152-refresh-eliminate-the-state-dataclass-and-stateyaml-file
@jeremyfowers jeremyfowers enabled auto-merge (squash) June 19, 2024 00:06
@jeremyfowers jeremyfowers merged commit 2943010 into refresh Jun 19, 2024
10 checks passed
@jeremyfowers jeremyfowers deleted the 152-refresh-eliminate-the-state-dataclass-and-stateyaml-file branch June 19, 2024 00:27
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.

3 participants