Skip to content

[Train] Fix HF Trainer with DatasetIterator, handle device_map - #32955

Merged
Yard1 merged 9 commits into
ray-project:masterfrom
Yard1:hf_get_count_from_dataset_iterator
Mar 7, 2023
Merged

[Train] Fix HF Trainer with DatasetIterator, handle device_map#32955
Yard1 merged 9 commits into
ray-project:masterfrom
Yard1:hf_get_count_from_dataset_iterator

Conversation

@Yard1

@Yard1 Yard1 commented Mar 1, 2023

Copy link
Copy Markdown
Member

Why are these changes needed?

This PR switches to the DatasetIterator in Hugging Face Trainer code to avoid deprecation warnings and makes sure that the device_map argument in HuggingFacePredictor is handled properly.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Yard1 added 6 commits March 1, 2023 22:05
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
@Yard1 Yard1 changed the title [Train] Fix HF Trainer with DatasetIterator [Train] Fix HF Trainer with DatasetIterator, handle device_map Mar 7, 2023
@Yard1
Yard1 marked this pull request as ready for review March 7, 2023 01:05
@Yard1
Yard1 requested review from amogkam, bveeramani and krfricke March 7, 2023 01:05

@bveeramani bveeramani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread python/ray/train/huggingface/huggingface_predictor.py
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>

try:
dataset_length = dataset.count()
dataset_length = dataset._base_dataset_or_pipeline.count()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With datasets streaming by default, count() will trigger full execution of the dataset.

Is length required here, or can we go with the pipelined case as the default?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the length would make for best UX. How about we discuss that in a followup?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok can we not use private attributes from DatasetIterator?

@Yard1 Yard1 Mar 7, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I obtain the count in that case? I don't want to have to make breaking changes in HuggingFaceTrainer in this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that's fine, but let's make sure to follow up

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make an issue

Yard1 added 2 commits March 7, 2023 19:26
Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
@Yard1
Yard1 merged commit 6c20bc6 into ray-project:master Mar 7, 2023
@Yard1
Yard1 deleted the hf_get_count_from_dataset_iterator branch March 7, 2023 21:41
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request Mar 21, 2023
…ay-project#32955)

This PR switches to the DatasetIterator in Hugging Face Trainer code to avoid deprecation warnings and makes sure that the device_map argument in HuggingFacePredictor is handled properly.

---------

Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
…ay-project#32955)

This PR switches to the DatasetIterator in Hugging Face Trainer code to avoid deprecation warnings and makes sure that the device_map argument in HuggingFacePredictor is handled properly.

---------

Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
…ay-project#32955)

This PR switches to the DatasetIterator in Hugging Face Trainer code to avoid deprecation warnings and makes sure that the device_map argument in HuggingFacePredictor is handled properly.

---------

Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
…ay-project#32955)

This PR switches to the DatasetIterator in Hugging Face Trainer code to avoid deprecation warnings and makes sure that the device_map argument in HuggingFacePredictor is handled properly.

---------

Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
…ay-project#32955)

This PR switches to the DatasetIterator in Hugging Face Trainer code to avoid deprecation warnings and makes sure that the device_map argument in HuggingFacePredictor is handled properly.

---------

Signed-off-by: Antoni Baum <antoni.baum@protonmail.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants