[integration] Update Ray Tune integration for Ray 2.7 #26499

justinvyu · 2023-09-29T23:55:06Z

What does this PR do?

Ray 2.7 introduced some backwards-incompatible API changes, which broke the HuggingFace transformers integration with Ray Tune trainer.hyperparameter_search(backend="ray"). This PR fixes the integration to use the new APIs. Note that this means that the next transformers version will no longer support ray<2.7.

I have manually tested this with the example in the Ray repo here: ray-project/ray#40125
This will be regularly tested in CI once this PR is in.

ray-project/ray#39763

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@richardliaw

Signed-off-by: Justin Yu <[email protected]>

src/transformers/hyperparameter_search.py

justinvyu · 2023-10-02T20:56:46Z

src/transformers/integrations/integration_utils.py

-    if "keep_checkpoints_num" in kwargs and kwargs["keep_checkpoints_num"] > 0:
-        # `keep_checkpoints_num=0` would disabled checkpointing
-        trainer.use_tune_checkpoints = True
-        if kwargs["keep_checkpoints_num"] > 1:
-            logger.warning(
-                f"Currently keeping {kwargs['keep_checkpoints_num']} checkpoints for each trial. "
-                "Checkpoints are usually huge, "
-                "consider setting `keep_checkpoints_num=1`."
-            )


Previously, you needed to set a # of checkpoints to keep in order to do checkpointing at all -- now this is just an optional arg

src/transformers/integrations/integration_utils.py

krfricke

Can we confirm that restoration works correctly? Are there tests for this?

src/transformers/hyperparameter_search.py

krfricke · 2023-10-03T06:44:57Z

src/transformers/integrations/integration_utils.py

+            with checkpoint.as_directory() as checkpoint_dir:
+                for subdir in os.listdir(checkpoint_dir):
+                    if subdir.startswith(PREFIX_CHECKPOINT_DIR):
+                        checkpoint_path = os.path.join(checkpoint_dir, subdir)


Doesn't the checkpoint_dir get removed after getting out of scope?

I think we need checkpoint.to_directory() here instead

Good catch, fixed and tested restoration with an updated test (in the ray repo which I'll link)

src/transformers/integrations/integration_utils.py

…to update_ray_integration

Signed-off-by: Justin Yu <[email protected]>

…to update_ray_integration

Signed-off-by: Justin Yu <[email protected]>

justinvyu · 2023-10-25T21:56:26Z

Tagging @muellerzr @pacman100 -- who can help get this PR in? Thanks!

ArthurZucker

Thanks for the fix 🤗 ! I guess TrainerHyperParameterRayIntegrationTest is the test that was broken in transformers. Looks good to me, pinging @pacman100 and @muellerzr to have a second look!

ArthurZucker · 2023-11-20T06:35:54Z

src/transformers/trainer.py

+        metrics = metrics.copy()
+        self.objective = self.compute_objective(metrics)


No sure I understand this change

Oh, this is because the metrics get modified later with some extra keys, so I want to keep the original one in tact: https://github.com/huggingface/transformers/pull/26499/files/9fbd0ec915455293eb4a1730e6a473d5c11b1151#diff-ed55888e6665791fe92cc8fc0c499da54f4ace6738551cd9a2591881cda076deR1199

muellerzr

Overall LG2M, agree though that perhaps leave it as a one-liner instead of splitting it into 2 :)

pacman100

Thank you @justinvyu for updating ray tune integration! 🚀✨

muellerzr · 2023-11-20T16:21:26Z

@justinvyu FYI seems like there's still some errors/issues with this: #27598

justinvyu · 2023-11-20T17:54:31Z

Ah thanks for confirming the fix with the user, I'll take a look at that.

justinvyu · 2023-11-29T20:25:30Z

@muellerzr I took a look at the user issue, and it's actually something in the example that should be changed. The limitation of the integration is that non-serializable objects like (metrics) cannot be supplied to the HF trainer. This PR is good to merge though.

muellerzr · 2023-12-05T00:12:10Z

@justinvyu a quick make style; make quality; should fix the failures then we should be set and good :) Thanks for your patience!

Signed-off-by: Justin Yu <[email protected]>

muellerzr

Thanks! Failures look to be unrelated? cc @ArthurZucker to make sure

HuggingFaceDocBuilderDev · 2023-12-05T14:04:17Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

ArthurZucker

Changes are unrelated indeed feel free to rebase and I'll merge then!

ArthurZucker · 2023-12-09T10:04:02Z

The test is also failing on main, merging 😉

justinvyu added 3 commits September 29, 2023 12:51

fix tune integration for ray 2.7+

0af114d

Signed-off-by: Justin Yu <[email protected]>

add version check for ray tune backend availability

d2f04cf

Signed-off-by: Justin Yu <[email protected]>

missing import

d4beffc

Signed-off-by: Justin Yu <[email protected]>

justinvyu commented Oct 2, 2023

View reviewed changes

src/transformers/hyperparameter_search.py Outdated Show resolved Hide resolved

justinvyu commented Oct 2, 2023

View reviewed changes

src/transformers/integrations/integration_utils.py Outdated Show resolved Hide resolved

krfricke reviewed Oct 3, 2023

View reviewed changes

justinvyu added 6 commits October 4, 2023 14:06

Merge branch 'main' of https://github.com/huggingface/transformers in…

0b808c7

…to update_ray_integration

pin min version instead

288a631

Signed-off-by: Justin Yu <[email protected]>

address comments

751dfe5

Signed-off-by: Justin Yu <[email protected]>

some fixes

e5bb092

Signed-off-by: Justin Yu <[email protected]>

fix unnecessary final checkpoint

1d1e298

Signed-off-by: Justin Yu <[email protected]>

fix lint

9069ddf

Signed-off-by: Justin Yu <[email protected]>

krfricke mentioned this pull request Oct 12, 2023

[integration] Update Ray Tune integration for Ray 2.7 (continuation) #26749

Closed

5 tasks

justinvyu added 2 commits October 25, 2023 12:31

Merge branch 'main' of https://github.com/huggingface/transformers in…

058a04a

…to update_ray_integration

dep table fix

9fbd0ec

Signed-off-by: Justin Yu <[email protected]>

huggingface deleted a comment from github-actions bot Nov 20, 2023

ArthurZucker requested a review from pacman100 November 20, 2023 06:32

ArthurZucker approved these changes Nov 20, 2023

View reviewed changes

pacman100 mentioned this pull request Nov 20, 2023

Hyperparameter search error with Ray tune #27598

Closed

4 tasks

muellerzr approved these changes Nov 20, 2023

View reviewed changes

This was linked to issues Nov 20, 2023

Ray Release 2.7.0 breaks Trainer.hyperparameter_search #26314

Closed

Hyperparameter search error with Ray tune #27598

Closed

pacman100 approved these changes Nov 20, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into update_ray_integration

e95673a

fix lint

560f7d3

Signed-off-by: Justin Yu <[email protected]>

muellerzr approved these changes Dec 5, 2023

View reviewed changes

ArthurZucker approved these changes Dec 6, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into update_ray_integration

db4c491

ArthurZucker merged commit 5fa66df into huggingface:main Dec 9, 2023
20 of 22 checks passed

justinvyu deleted the update_ray_integration branch December 14, 2023 22:36

justinvyu mentioned this pull request Dec 14, 2023

Ray Tune external integration with HuggingFace Transformers (trainer.hyperparameter_search) is broken in Ray 2.7+ ray-project/ray#39763

Closed

justinvyu mentioned this pull request Feb 14, 2024

Ray Release 2.7.0 breaks Trainer.hyperparameter_search #26314

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[integration] Update Ray Tune integration for Ray 2.7 #26499

[integration] Update Ray Tune integration for Ray 2.7 #26499

justinvyu commented Sep 29, 2023 •

edited

Loading

justinvyu Oct 2, 2023

krfricke left a comment

krfricke Oct 3, 2023

justinvyu Oct 4, 2023

justinvyu commented Oct 25, 2023

ArthurZucker left a comment

ArthurZucker Nov 20, 2023

justinvyu Nov 20, 2023

muellerzr left a comment

pacman100 left a comment

muellerzr commented Nov 20, 2023

justinvyu commented Nov 20, 2023

justinvyu commented Nov 29, 2023

muellerzr commented Dec 5, 2023

muellerzr left a comment

HuggingFaceDocBuilderDev commented Dec 5, 2023

ArthurZucker left a comment

ArthurZucker commented Dec 9, 2023

		metrics = metrics.copy()
		self.objective = self.compute_objective(metrics)

[integration] Update Ray Tune integration for Ray 2.7 #26499

[integration] Update Ray Tune integration for Ray 2.7 #26499

Conversation

justinvyu commented Sep 29, 2023 • edited Loading

What does this PR do?

Before submitting

Who can review?

justinvyu Oct 2, 2023

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

krfricke Oct 3, 2023

Choose a reason for hiding this comment

justinvyu Oct 4, 2023

Choose a reason for hiding this comment

justinvyu commented Oct 25, 2023

ArthurZucker left a comment

Choose a reason for hiding this comment

ArthurZucker Nov 20, 2023

Choose a reason for hiding this comment

justinvyu Nov 20, 2023

Choose a reason for hiding this comment

muellerzr left a comment

Choose a reason for hiding this comment

pacman100 left a comment

Choose a reason for hiding this comment

muellerzr commented Nov 20, 2023

justinvyu commented Nov 20, 2023

justinvyu commented Nov 29, 2023

muellerzr commented Dec 5, 2023

muellerzr left a comment

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Dec 5, 2023

ArthurZucker left a comment

Choose a reason for hiding this comment

ArthurZucker commented Dec 9, 2023

justinvyu commented Sep 29, 2023 •

edited

Loading