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

add upload_store as prepare parameter #135

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

Kenneth-Schroeder
Copy link
Contributor

@Kenneth-Schroeder Kenneth-Schroeder commented Dec 6, 2024

Hi all,
this will add upload_store as parameter to the prepare function to make it possible to specify this yaml entry through the step's .py file.

Example:

valohai.prepare(
    step="training-step",
    image="docker.io/myorg/training:1.0",
    default_inputs=default_inputs,
    default_parameters=default_parameters,
    upload_store="abc123de-f456-7890-ghij-klmnopqrstuv"  # UUID of your upload store
)

You will need to bump the version as well and set it in valohai-cli.

Flow

  1. Entry Point: valohai.prepare(upload_store="...")

    • Initial value is provided as a parameter to prepare()
    • This is the user's entry point for configuration
  2. Global State Storage: global_state.py

    • The value is stored in global_state.upload_store
    • Acts as a temporary storage during the execution
  3. AST Processing: PrepareParser.process_valohai_prepare_call()

    • Parses the Python code using AST (Abstract Syntax Tree)
    • Identifies the upload_store parameter in the prepare() call
    • Extracts its value using ast.literal_eval()
  4. Parse Result Creation: ParseResult namedtuple

    • Packages the parsed information into a structured format
    • Includes upload_store alongside other configuration values like step name, parameters, etc.
  5. Config Source Parsing: parse_config_from_source()

    • Receives the ParseResult from the previous step
    • Extracts the upload_store value for config generation
  6. Config Generation: generate_config()

    • Takes upload_store as a parameter
    • Prepares it for step generation
    • Coordinates the overall config creation process
  7. Step Generation: generate_step()

    • Receives upload_store value from generate_config
    • Uses it in Step object creation
    • Handles other step configuration details
  8. Step Class Storage: Step class

    • Stores upload_store as an instance variable
    • Manages it alongside other step properties
  9. Serialization: Step.serialize()

    • Converts Python-style upload_store to YAML-style upload-store
    • Prepares the value for YAML output
    • Follows YAML conventions for naming
  10. Final Output: YAML file

    • Contains the final upload-store field with hyphenated name

Addition:

Please also update the documentation for visibility of this feature or point me to the docs repo (I couldn't find a public one).

Also, please mention in the documentation how to specify parameter descriptions. This is not well documented yet either.
Example:

default_params = {
    "parameter_name": {
        "default": "default_value",
        "description": "helpful description here",
        "optional": True,
        "type": "string"
    },
    ...
}

@ruksi ruksi requested a review from akx December 10, 2024 10:03
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

Looks straightforward enough. Thanks!

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

Oop, actually: tests & lint is failing

@Kenneth-Schroeder
Copy link
Contributor Author

pre-commit and tests are green for me (now)

@Kenneth-Schroeder
Copy link
Contributor Author

I would suggest an update to make tests compatible with newer python versions than 3.7. There seem to be slight differences in default value serialization, causing tests to fail when running on envs with python 3.11, for example. You would either need to update the expected results for the tests or make some small changes to how parameters with default values are serialized in valohai-yaml.

@Kenneth-Schroeder Kenneth-Schroeder requested a review from akx January 9, 2025 14:54
@Kenneth-Schroeder
Copy link
Contributor Author

FYI if you are seeing
TypeError: __init__() got an unexpected keyword argument 'upload_store' (init of Step class from valohai-yaml)
this is probably caused by your automated tests running on python 3.7 which won't use the latest valohai-yaml version.

@akx akx force-pushed the feature/prepare_upload_store branch from a6fcc50 to 1c45400 Compare January 15, 2025 16:04
@akx akx merged commit 2d80e53 into valohai:master Jan 15, 2025
5 checks passed
@akx
Copy link
Member

akx commented Jan 15, 2025

@Kenneth-Schroeder Thanks for the comments re CI! valohai-utils will require Python 3.8+ going forward, and valohai-yaml was also fixed to be less silly about (re)serialization.

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