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

[AIR] Improve to_air_checkpoint with path #26532

Merged
merged 3 commits into from
Jul 14, 2022

Conversation

Yard1
Copy link
Member

@Yard1 Yard1 commented Jul 13, 2022

Signed-off-by: Antoni Baum [email protected]

Why are these changes needed?

Clarifies the API for to_air_checkpoint.

Related issue number

Checks

  • 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 Yard1 added this to the Ray AIR milestone Jul 13, 2022
Copy link
Contributor

@xwjiang2010 xwjiang2010 left a comment

Choose a reason for hiding this comment

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

Thanks @Yard1! Makes sense as a UX improvement. I have the same concern as the other PR - i.e. the semantics is not exactly clear as to who should be responsible for cleaning the "temporary" directories up.
I like your idea of a context manager, but not sure how that would work for other to_air_checkpoint (e.g. those not relying on a path) - maybe a no-op.
Ok to leave as a future followup.

Copy link
Contributor

@xwjiang2010 xwjiang2010 left a comment

Choose a reason for hiding this comment

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

The current semantic looks good - this way user at least supplies a path and they should be clean it up :)

@richardliaw richardliaw merged commit 4273d22 into ray-project:master Jul 14, 2022
xwjiang2010 pushed a commit to xwjiang2010/ray that referenced this pull request Jul 19, 2022
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
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