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

Load config history #472

Merged
merged 17 commits into from
Mar 7, 2023
Merged

Load config history #472

merged 17 commits into from
Mar 7, 2023

Conversation

JosephMarinier
Copy link
Contributor

@JosephMarinier JosephMarinier commented Mar 2, 2023

Description:

make ARTIFACT_PATH=cache REJECTION_CLASS=NO_INTENT DATASET='{
    "class_name": "loading_resources.load_CLINC150_data",
    "remote": "'$PWD'/azimuth_shr",
    "kwargs": {
        "python_loader": "'$PWD'/azimuth_shr/data/CLINC150.py",
        "full_path": "'$PWD'/azimuth_shr/files/clinc-demo/oos-eval/data/clinc_data_dummy.json"
    }
}' launch.local

image

Checklist:

You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.

  • ISSUE NUMBER. You linked the issue number (Ex: Resolve #XXX).
  • PRE-COMMIT. You ran pre-commit on all commits, or else, you
    ran pre-commit run --all-files at the end.
  • USER CHANGES. The changes are added to CHANGELOG.md and the documentation, if they impact
    our users.
  • DEV CHANGES.
    • Update the documentation if this PR changes how to develop/launch on the app.
    • Update the README files and our wiki for any big design decisions, if relevant.
    • Add unit tests, docstrings, typing and comments for complex sections.

@JosephMarinier JosephMarinier self-assigned this Mar 2, 2023
@JosephMarinier JosephMarinier force-pushed the joseph/load-config-history branch 2 times, most recently from 44ab1ca to 859a96b Compare March 2, 2023 06:41
@JosephMarinier JosephMarinier marked this pull request as ready for review March 2, 2023 06:42
defaulting to loading config from env vars only
so I can make a dummy release and try the docker image.
Copy link
Contributor

@gabegma gabegma left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to make a clear PR. I was hesitant, but I see it more clearly now. I like it. So the default is to ignore the history, right? So no confusion if someone does make CFG_PATH='bla.json' launch; they will see 'bla' in Azimuth, no matter the config history.

azimuth/config.py Show resolved Hide resolved
docs/docs/development/launching.md Show resolved Hide resolved
docs/docs/development/launching.md Outdated Show resolved Hide resolved
docs/docs/development/launching.md Outdated Show resolved Hide resolved
@@ -65,6 +65,22 @@ From this point, the back end will launch and compute the start-up task.
* You can consult the openapi documentation at `localhost:8091/docs`. From there, you can consult the API documentation and try out the different endpoints. This can be useful for debugging.
* Note that the back end will not reload automatically based on code changes.

After a successful start, Azimuth saves the provided config in its `config_history.jsonl` artifact. If you use the API to edit the config, the edits are saved there. If you restart Azimuth (for example after shutting it down for the night), you can resume where you left off with `LOAD_CONFIG_HISTORY=1`. In order to find the right `config_history.jsonl`, Azimuth needs the same `ARTIFACT_PATH`, so to resume the above example, it needs to be specified:
```shell
make ARTIFACT_PATH=cache LOAD_CONFIG_HISTORY=1 launch.local
Copy link
Contributor Author

@JosephMarinier JosephMarinier Mar 2, 2023

Choose a reason for hiding this comment

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

In our Python code, we could consider making "cache" the default artifact path, useful locally, rather than "/cache", useful in docker, and set it to "/cache" in our Dockerfile. Tha would simplify:

Suggested change
make ARTIFACT_PATH=cache LOAD_CONFIG_HISTORY=1 launch.local
make LOAD_CONFIG_HISTORY=1 launch.local

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like a good idea

Copy link
Contributor Author

@JosephMarinier JosephMarinier Mar 3, 2023

Choose a reason for hiding this comment

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

Done in bf8861a

I like it. It also makes the documentation more straightforward; a simpler description of the "normal" use case with the default artifact_path, and then a warning box about customizing artifact_path.

image

relying on our (already existing) code to validate that the dataset is defined after loading the final config.
@JosephMarinier JosephMarinier marked this pull request as draft March 3, 2023 20:51
azimuth/config.py Outdated Show resolved Hide resolved
docs/docs/development/launching.md Show resolved Hide resolved
…oad-config-history

* commit '674585e9bdfd543771ee74b407059b41fc979f24':
  Clean up docker image (#316)
  Validate config min/max values (#474)
  Bump werkzeug from 2.1.0 to 2.2.3 (#443)
  Bump dns-packet from 5.3.1 to 5.4.0 in /webapp (#473)
  Update tensorflow (#355)
  Bump json5 from 1.0.1 to 1.0.2 in /webapp (#359)
  Fix negative query params (#471)
@JosephMarinier JosephMarinier marked this pull request as ready for review March 6, 2023 22:21
Copy link
Contributor

@gabegma gabegma left a comment

Choose a reason for hiding this comment

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

LGTM!! Awesome new feature, great work!

since changing the default `artifact_path` to `"cache"` made that unnecessary.
@JosephMarinier JosephMarinier enabled auto-merge (squash) March 7, 2023 17:42
@JosephMarinier JosephMarinier merged commit 6cfd784 into main Mar 7, 2023
@JosephMarinier JosephMarinier deleted the joseph/load-config-history branch March 7, 2023 18:01
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