Cache latest version per dataset #528
Conversation
dmannarino
requested review from
solomon-negusse,
gtempus,
danscales,
jterry64 and
manukala6
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #528 +/- ##
===========================================
+ Coverage 81.70% 81.72% +0.01%
===========================================
Files 125 125
Lines 5615 5619 +4
===========================================
+ Hits 4588 4592 +4
Misses 1027 1027
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
app/crud/versions.py
Outdated
| @@ -100,6 +99,18 @@ async def create_version(dataset: str, version: str, **data) -> ORMVersion: | |||
| ) | |||
| new_version.metadata = metadata | |||
|
|
|||
| # FIXME: Should we really allow one to specify a new version as | |||
| # latest on creation? I thought we didn't. Seems like it will cause | |||
| # requests to temporarily go to a perhaps incompletely-imported | |||
There was a problem hiding this comment.
If you look at all the callers, it is only used for testing (see test_versions.py::test_latest_versions). So, how about you remove the FIXME but add to the function comment that data.is_latest is only allowed to be true for testing. (You;ll see is_latest is not a valid arg in the version-create route.)
There was a problem hiding this comment.
Ahh, okay, thanks for figuring that out! Will add the note.
app/crud/versions.py
Outdated
| # f"Setting version {version} to latest for dataset {dataset}. " | ||
| # f"Cache info: {get_latest_version.cache_info()}" | ||
| # ) | ||
| _: bool = get_latest_version.cache_invalidate(dataset) |
There was a problem hiding this comment.
Seems like it might make sense to put the cache_invalidate call in _reset_is_latest, so we don't forget to call it when we call reset_is_latest?
There was a problem hiding this comment.
Yeah, I'll do that. I was on the fence about it, but now that you say it, that seems better.
app/crud/versions.py
Outdated
| @@ -155,6 +171,12 @@ async def _update_is_downloadable( | |||
|
|
|||
|
|
|||
| async def _reset_is_latest(dataset: str, version: str) -> None: | |||
| """Set is_latest to False for all other versions of a dataset.""" | |||
| # TODO: Should we make sure the version is valid to avoid setting nothing | |||
| # to latest? Or is being able to do that a desired feature? | |||
There was a problem hiding this comment.
I think you can remove this TODO, and just add a line to the header comment that says it is recommended to set is_latest flag for version before calling this method to reset is_latest for all other versions [since that is what is done in update_version].
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information