-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
🧰♻️ Add Dependency Caching #1239
Conversation
The GHA runner does not have a GPU anyway
BatchCompilationTest was failing before, but is not needed anyway
# fixme: bring this upstream to class_resolver.contrib? | ||
lr_scheduler_resolver = ClassResolver.from_subclasses(LRScheduler, default=ExponentialLR, suffix="LR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cthoyt this could be fixed in class-resolver
upstream; we could also think about adding some logic to make this work in torch>=2.0
and torch < 2
, though I am not sure whether to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the idea of adding more contrib to class-resolver. If it's no so hard to make compatibility, then great, otherwise we should aim for only supporting the latest version
# fixme: find reason / enforce single-thread | ||
@unittest.skipIf(condition=True, reason="Problems with multi-processing") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't resolve this issue, and this PR/test case failure seems to block others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine for me
# - name: Check RST format | ||
# run: tox -e doclint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defined in tox, but not used in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough for now, thanks for looking into all of these corner cases
@mberr we can do follow-up to fix the class-resolver contrib. We might also have to change the "required" tests configuration for the protections on the master branch, but then this is good from my side to merge. |
With torch 2.0, the (private) `_LRScheduler` class has been transformed to a public `LRScheduler` class. cf. pykeen/pykeen#1239
This PR enables caching for dependencies installed via pip, cf. https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows
It also updates the code to be torch 2.0 compliant.