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

Memoizer: ask any ImageReader instances to remove all but the current reader #4254

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

melissalinkert
Copy link
Member

Ported from a private draft, for wider discussion (cc @joshmoore). glencoesoftware/omero-ms-image-region#148 provides some additional context for where this idea is coming from.

To minimally demonstrate the difference, run something like showinf -cache -minmax -expand -separate without this change, and note size of memo file (in same directory as data). Run the same thing twice with this change - once to generate the new memo file (note the size again), and once after a minute or so to verify that the new memo file is used and not overwritten. Note -minmax -expand -separate mimics the reader stack used in OMERO.

However, this has the potential to impact anything that uses Memoizer, including but not limited to OMERO. So ideas of what might break, why this change might be bad in general, or other approaches to consider would all be welcome.

One potential problem identified is what happens when there is an existing memo file generated by this change, but new reader (or better type detection) is introduced such that the underlying reader that should be used for the file changes.

@joshmoore
Copy link
Member

why this change might be bad in general

:) Nope. I'm generally supportive. I remember having considered this, but probably didn't spend the time to think it through well enough as you have.

but new reader (or better type detection) is introduced such that the underlying reader that should be used for the file changes.

This is a good and interesting point, but I imagine that a similar edge case could be found even without dropping instances from ImageReader. Since only a breaking change in the storage layout (new field) would trigger an invalidation, even just a change to the logic that fills a field could actually be something should invalidate the cache.

other approaches to consider

Combined with the previous point, the only additional option I can think of is that we more fully define the semantics of when VERSION should be bumped.

@melissalinkert
Copy link
Member Author

Thanks, @joshmoore.

Combined with the previous point, the only additional option I can think of is that we more fully define the semantics of when VERSION should be bumped.

That's a good point. Maybe at minimum, adding a new reader means updating the version number?

@joshmoore
Copy link
Member

No immediate objections to that other than that it might lead to a bit more churn then we've had to date. On the flip side, we can't necessarily control who has added a reader to ImageReader at runtime. I could imagine additionally allowing OMERO to override the version itself (passing a parameter? environment variable?) since that's a more "controlled" environment, but the general case in B-F land is definitely tricky.

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.

2 participants