[trainer / deepspeed] fix hyperparameter_search #16740
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
| @@ -363,6 +363,32 @@ def test_stage3_nvme_offload(self): | |||
| trainer.train() | |||
| self.assertIn("DeepSpeed info", cl.out, "expected DeepSpeed logger output but got none") | |||
|
|
|||
| def test_hyperparameter_search(self): | |||
There was a problem hiding this comment.
Doesn't this test require one of the HP search lib?
There was a problem hiding this comment.
That's a good question, I didn't need to install anything, but it doesn't mean something else didn't install what was required:
would this work?
diff --git a/setup.py b/setup.py
index 4d386ae00..a7bf8449f 100644
--- a/setup.py
+++ b/setup.py
@@ -288,6 +288,7 @@ extras["testing"] = (
)
+ extras["retrieval"]
+ extras["modelcreation"]
+ + extras["integrations"]
)
extras["quality"] = deps_list("black", "isort", "flake8", "GitPython", "hf-doc-builder")
which would add:
extras["integrations"] = extras["optuna"] + extras["ray"] + extras["sigopt"]
to testing and thus have all the deps in place.
if we don't automate this and there is a skip rule then the test won't run which is an issue too.
There was a problem hiding this comment.
No we don't want to run the HP search tests at each PRs, so they shouldn't be in "testing". They are manually installed on one of the scheduled jobs. There are already require_optuna/require_ray/require_sigopt flags you can reuse.
There was a problem hiding this comment.
I hear you, but what's the point of having a test if it is not going to be run since its prerequisites won't be installed on CI?
What am I missing?
There was a problem hiding this comment.
oh, I see, I have to add the specific prerequisite install just for deepspeed tests. got it.
So just need to figure out which is it
There was a problem hiding this comment.
We have more than 10,000 tests and they don't run at each commit. That doesn't prevent us from adding more tests.
The regular CI job as well as the dev install do not install any of the integrations (wandb, neptuneAi, MThe regular CI job as well as the dev install do not install any of the integrations (wandb, neptuneAi, MLFlow, cometML, Ray, Optuna, Sigopt, and I probably forgot some) to avoid cluttering the testing env. I think you were very unhappy to have some of those installed in your envs for instance ;-)
Also, the maintenance of those integrations is not our responsibility but the one of each of those libraries maintainers. Running the corresponding tests as scheduled is enough to see if we accidentally broke something.
There was a problem hiding this comment.
yes, this makes total sense, Sylvain.
I figured out what you meant in the first place!
I wonder if it'd be easier to have for each "integration group" 2 targets:
pip install -e .[deepspeed]
pip install -e .[deepspeed-testing]
the first installing the deps for the integration to work, the second installing the deps for the tests for that integration to work. e.g. deepspeed doesn't care for optuna, but its tests do.
that way it'd be much simpler to manage CI configs as currently they are plastered with a lot of copy-n-paste and need to remember to update it in multiple workflow files.
so when adding new testing dependencies only setup.py will need to be updated.
There was a problem hiding this comment.
We could add that deepspeed-testing group, yes.
There was a problem hiding this comment.
ok, deepspeed-testing group pushed, please have a look before I merge. Thank you!
* [trainer / deepspeed] fix hyperparameter_search * require optuna * style * oops * add dep in the right place * create deepspeed-testing dep group * Trigger CI
This PR:
hyperparameter_searchdeepspeed config reset fix up that got out of sync with the normal code pathdeepspeed-testing@sgugger
Fixes: #11966 (comment)