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

Using GFile for persistent compilation caching in JAX #10771

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

sshahrokhi
Copy link
Collaborator

@sshahrokhi sshahrokhi commented May 19, 2022

The filesystem method that was used for persistent compilation caching, was chosen because it was able to implement Least Recently Used, but it didn't support gcs. GFile supports gcs which can be used between machines but unfortunately it is not following the LRU policy. Therefore, there is no maximum memory we allow for the cache as well.

[TESTING] I tested this method by using this test, tested for both local file and a gcs bucket to make sure both works fine.

@skye skye self-requested a review May 19, 2022 21:19
@skye skye self-assigned this May 19, 2022
Copy link
Collaborator

@skye skye left a comment

Choose a reason for hiding this comment

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

A few high-level comments:

  • Please update the PR title and description to be more descriptive (see https://cbea.ms/git-commit/ for a long description of how to write a good commit message). This can also be the commit message once you squash everything before commit.
  • Also describe how you tested this change in the PR description, since we don't have automatic Cloud TPU presubmits :(
  • Delete the old implementation
  • Add this as a breaking change to the changelog: https://github.com/google/jax/blob/main/CHANGELOG.md

jax/experimental/compilation_cache/gfile_cache.py Outdated Show resolved Hide resolved
jax/experimental/compilation_cache/gfile_cache.py Outdated Show resolved Hide resolved
@skye
Copy link
Collaborator

skye commented May 23, 2022

Also look at the lint failures please

@sshahrokhi
Copy link
Collaborator Author

sshahrokhi commented May 23, 2022

Thanks for the reviews! I am working on them! Meanwhile:

I didn't remove the max_cache_size_bytes so that this change would not be breaking(only the logic behind the file system is changing)! So do you suggest removing it and also add this to changelog?

@skye
Copy link
Collaborator

skye commented May 23, 2022

I didn't remove the max_cache_size_bytes so that this change would not be breaking(only the logic behind the file system is changing)! So do you suggest removing it and also add this to changelog?

Yes. This is a breaking change, but I think it's better to raise an error than to silently stop working.

@sshahrokhi sshahrokhi changed the title Gfilecache Using GFile for persistent compilation caching in JAX May 25, 2022
@sshahrokhi
Copy link
Collaborator Author

Thanks for all the comments, could you please take a look now? @skye

@sshahrokhi sshahrokhi requested a review from skye May 27, 2022 01:35
CHANGELOG.md Outdated Show resolved Hide resolved
jax/experimental/compilation_cache/gfile_cache.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@skye skye left a comment

Choose a reason for hiding this comment

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

Almost there!!

@sshahrokhi sshahrokhi requested a review from skye June 6, 2022 17:49
Copy link
Collaborator

@skye skye left a comment

Choose a reason for hiding this comment

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

Looks great! Can you squash all your changes to a single commit and then I'll get this in?

@google-ml-butler google-ml-butler bot added kokoro:force-run pull ready Ready for copybara import and testing labels Jun 7, 2022
@sshahrokhi sshahrokhi force-pushed the gfilecache branch 3 times, most recently from bca98cb to f2a1b53 Compare June 7, 2022 00:43
import threading
import time

class FileSystemCacheTest(jtu.JaxTestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to GFileCacheTest

@skye
Copy link
Collaborator

skye commented Jun 10, 2022

Please squash your changes again too, and then we can get this in 🤞

@sshahrokhi
Copy link
Collaborator Author

done! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants