-
Couldn't load subscription status.
- Fork 1.3k
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]]): |
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.
@skshetry Hey sorry for the delay, the _update_params function you mentioned should now have extra no_hydra flag that bypasses entire composing process and goes straight to ordinary overrides application
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.
Hey @StaszekM, did you forget to push new changes?
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.
Hi, I checked twice and changes for this function are visible, maybe GitHub bugged or something.
I am looking at changes from all commits: https://github.com/iterative/dvc/pull/10866/files
here's the fragment from changed lines:
dvc/repo/experiments/queue/base.py
def _update_params(self, params: dict[str, list[str]], no_hydra: bool = False): #<--- new param
"""Update param files with the provided `Hydra Override`_ patterns.
Args:
params: Dict mapping paths to `Hydra Override`_ patterns,
provided via `exp run --set-param`.
no_hydra: Disable Hydra from automatically overwriting all params.
.. _Hydra Override:
https://hydra.cc/docs/advanced/override_grammar/basic/
"""
from dvc.utils.hydra import apply_overrides, compose_and_dump
logger.debug("Using experiment params '%s'", params)
hydra_config = self.repo.config.get("hydra", {})
hydra_enabled = hydra_config.get("enabled", False) and not no_hydra #<--- bypass conditionally
#...rest unchanged
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. π