-
Notifications
You must be signed in to change notification settings - Fork 1.2k
exp: add support for --no-hydra flag #10866
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
base: main
Are you sure you want to change the base?
Conversation
Introduces --no-hydra in the CLI of dvc exp run, which disables automatically inserting Hydra config into params.yaml. Fixes iterative#10863
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10866 +/- ##
==========================================
+ Coverage 90.68% 91.12% +0.43%
==========================================
Files 504 504
Lines 39795 40794 +999
Branches 3141 3239 +98
==========================================
+ Hits 36087 37172 +1085
+ Misses 3042 2982 -60
+ Partials 666 640 -26 β View full report in Codecov by Sentry. π New features to boost your workflow:
|
@pytest.mark.parametrize("no_hydra", [True, False]) | ||
def test_mixing_no_hydra_and_sweeps( | ||
tmp_dir, params_repo, dvc, mocker, hydra_enabled, overrides, expected, no_hydra | ||
): | ||
# Passing no_hydra should not prevent user from | ||
# queuing sweeps with --set-param and --queue | ||
patched = mocker.patch.object(dvc.experiments, "queue_one") |
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.
What's the point of using --no-hydra
here?
Would this still hydrate yaml
file using hydra? Or, is this only limited to sweeps?
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.
During first tests (dvc exp run
) no-hydra flag worked fine, but I noticed later that this flag does not work if we pass --queue
and related sweep params, then sweeps are created with configs hydrated with Hydra.
So I changed implementation in both exp scenarios (as one exp and as a sweep) to handle this new flag
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.
You may also need to change this:
dvc/dvc/repo/experiments/queue/base.py
Line 444 in 950e9c4
def _update_params(self, params: dict[str, list[str]]): |
Introduces --no-hydra in the CLI of dvc exp run, which disables automatically inserting Hydra config into params.yaml.
Fixes #10863
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. π