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.

Sign up for GitHub

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

Jump to bottom

Fixup: update cache key reference to requirements.txt (removed) #16

Closed
wants to merge 1 commit into from
Closed

Fixup: update cache key reference to requirements.txt (removed) #16

wants to merge 1 commit into from

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Jun 17, 2020 •
edited
Loading

I missed this reference to requirements.txt while developing #14; using setup.py as a dependency cache key should be relatively stable and maintains the property that the cache should be invalidated when dependencies are updated (although unfortunately it may also invalidate the cache during other unrelated changes)

May resolve #15

@seperman
Copy link
Owner

seperman commented Aug 1, 2020

Hi @jayaddison
Sorry I'm not sure how I missed this PR. Why do you think setup.py should be used for the signature instead of requirements.txt?

@jayaddison
Copy link
Contributor Author

Hey @seperman - no problem! Using setup.py in the key isn't ideal (because it contains non-dependency-related code and content), but requirements.txt was removed in #14 (due to the dynamic dependency selection) so I don't think it's suitable as a cache key.

Given that caching is a nice-to-have (i.e. lack of cache shouldn't cause failures, although it can waste time and resources), and that setup.py shouldn't change often -- and should always change when the dependency list does -- it seemed the next best option.

@jayaddison
Copy link
Contributor Author

Closing this as stale; there's a small chance it may resolve #15, but I don't know when I'd be likely to investigate that more thoroughly.

@jayaddison jayaddison closed this Dec 10, 2020
@seperman
Copy link
Owner

Ok thanks for closing the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CircleCi script is broken
2 participants
@jayaddison @seperman