Skip to content
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

[Train][transformers] Skip reporting checkpoint when get_latest_checkpoint returns None. #42953

Conversation

woshiyyya
Copy link
Member

@woshiyyya woshiyyya commented Feb 2, 2024

Why are these changes needed?

An OSS users found that RayTrainReportCallback fails to copy checkpoint while doing multi-node distributed training: #42926

Previously, transformers Trainer first creates a tmp dir for all workers to save checkpoint files, then renames the tmp-checkpoint-* dir to checkpoint-* on all workers. They recently introduced a behavior change, which forces it to only rename on global-rank-0 worker (if save_on_all_nodes=False). Therefore, except for node of rank 0 worker, the remaining nodes still keep the tmp-checkpoint-* dir around, and returns None in transformers.trainer.get_last_checkpoint since no checkpoint-* folders are found in the the args.output_dir (see here).

This PR modifies ray.train.transformers.RayTrainReportCallback to only report checkpoint when transformers.trainer.get_latest_checkpoint returns a valid path. This is to address a behavior change made in transformers starting in 4.37.0 where only the global rank 0 worker will rename the temporary checkpoint directory so that it is found by this get_latest_checkpoint function.

Update: This feature is introduced in the lastest transformers version, but we will not update the package version to avoid breaking other tests.

Related issue number

Closes: #42926

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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya woshiyyya changed the title [Train] Upgrade transformers version and report only when get_latest_checkpoint returns valid path. [Train][transformers] Skip reporting checkpoint when get_latest_checkpoint returns None. Feb 2, 2024
@woshiyyya woshiyyya marked this pull request as ready for review February 2, 2024 21:38
Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
@woshiyyya woshiyyya force-pushed the train/fix_transformers_checkpoint_callback branch from cb10e27 to f32943b Compare February 6, 2024 23:24
Signed-off-by: Yunxuan Xiao <yunxuanx@anyscale.com>
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

Thanks!

@justinvyu justinvyu merged commit b049e78 into ray-project:master Feb 13, 2024
9 checks passed
kevin85421 pushed a commit to kevin85421/ray that referenced this pull request Feb 17, 2024
…kpoint` returns `None` (ray-project#42953)

Modify `ray.train.transformers.RayTrainReportCallback` to only report checkpoint when `transformers.trainer.get_latest_checkpoint` returns a valid path. This is to address a behavior change made in transformers starting in `4.37.0` where only the global rank 0 worker will rename the temporary checkpoint directory so that it is found by this `get_latest_checkpoint` function.

---------

Signed-off-by: woshiyyya <xiaoyunxuan1998@gmail.com>
Signed-off-by: Yunxuan Xiao <yunxuanx@anyscale.com>
@viirya
Copy link

viirya commented Feb 25, 2024

Hi, thanks for fixing this. I also encountered this bug today with ray 2.9.2. When I tried to fix it and just found it is already fixed in latest branch.

But I just checked ray 2.9.3 which is released 2 days ago. This fix seems not included there. Actually this commit looks like merged in 2/13, it should be in the time range of ray 2.9.2 - ray 2.9.3 (ray-2.9.2...ray-2.9.3, where latest commit is 2/15).

Any idea why this fix isn't included and when a new release will include this fix? Thanks.

@woshiyyya
Copy link
Member Author

@viirya This will be included in 2.10 (release in the next month). You can use our nightly build if you want to include this fix asap.

@viirya
Copy link

viirya commented Feb 25, 2024

Got it. Thank you @woshiyyya

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.

[Train] Transformer's RayTrainReportCallback failed to copy checkpoints
4 participants