Validate checkpoint against inference environment #201
Conversation
GitHub Actions
Failure
GitHub Actions
Error
|
Link to ReadTheDocs sample build for this PR can be found at: |
| if self.config.get("validate", True): | ||
| yield self.valid_checkpoint(ckpt_path) | ||
| else: | ||
| yield self._checkpoint(ckpt_path) |
There was a problem hiding this comment.
Validate the checkpoint if it is set to true or if validate: is not provided, otherwise just check that the checkpoint path exists, as we previously did for all runs.
There was a problem hiding this comment.
Is valid_checkpoint intentionally public so that it could be selected externally as one of the driver's tasks? I noticed the difference between this and the private _checkpoint.
There was a problem hiding this comment.
Yes. I would think it'll be fairly common to use previously trained checkpoints, and would be helpful for users to have a task for, whereas checking if the file path exists seems a bit odd to have as a task.
| }, | ||
| "validate": { | ||
| "type": "boolean" |
There was a problem hiding this comment.
Optional value.
GitHub Actions
Error
| if self.config.get("validate", True): | ||
| yield self.valid_checkpoint(ckpt_path) | ||
| else: | ||
| yield self._checkpoint(ckpt_path) |
There was a problem hiding this comment.
Is valid_checkpoint intentionally public so that it could be selected externally as one of the driver's tasks? I noticed the difference between this and the private _checkpoint.
| assert not cfgfile.is_file() | ||
| driverobj.anemoi_config() | ||
| with patch.object(driverobj, "valid_checkpoint", return_value=None): | ||
| driverobj.anemoi_config() |
There was a problem hiding this comment.
Is this meant to prevent valid_checkpoint from becoming part of the task graph, necessary because the new validate item is missing from the test-fixture config and missing implies True? If so, would something like
driverobj._config["validate"] = False
also work? This test has some driverobj._config tweaks a few lines above.
Same question applies to the update in test_anemoi_config__explicit_checkpoint, below.
EDIT: I now see that you're doing = False in test_anemoi_config__validate_false further down, so maybe I don't understand the intent of the mock in these two tests.
There was a problem hiding this comment.
The intention was for this function to test the default case: validate: true, but to mock it so that anemoi-inference doesn't check the validity of a dummy checkpoint, which would always fail. We can't maintain checkpoints in the repo for testing purposes, so this seemed like the best option at the time. The same would apply to test_anemoi_config__explicit_checkpoint.
test_anemoi_config__validate_false is meant to test the exception, and a test is needed to cover that exception for branch coverage.
github-actions
Bot
added
ci-running
GitHub Actions
Inactive
github-actions
Bot
added
ci-running
maddenp-cu
left a comment
maddenp-cu
left a comment
There was a problem hiding this comment.
It looks like the current CI failure might be a transient upstream issue that a re-run will resolve.
GitHub Actions
Inactive
github-actions
Bot
added
ci-running
|
@maddenp-cu Do you know what branch protection rule I'm breaking? I can bypass the requirement manually, but I'm wondering if everything was set up correctly. This is a branch from the main repo, not my fork, which to my understanding we are moving to, but aren't there yet. |
@elcarpenterNOAA Are you clear to merge now? There was a too-strict prohibition on updating |
|
@maddenp-cu Yes, thank you! And my apologies: I forgot I also had the ability to edit branch protection rules. |
2 participants
Description:
Fixes #146.
Type of change:
Area(s) affected
Commit Requirements:
Testing / Verification: