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

Ensure cache coherence for views of time instances #15453

Merged
merged 2 commits into from Oct 27, 2023
Merged

Ensure cache coherence for views of time instances #15453

merged 2 commits into from Oct 27, 2023

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Oct 9, 2023 •
edited

Description

This pull request ensures that the Time caches of formats and scales do not get out of sync with the actual data, even if another instance, holding a view of the data is written to. E.g., if one does t01 = t[:2], and sets t[0] after, it is now guaranteed that t01.value will correctly reflect that change in value.

Fixes #15452

@taldcroft - this was considerably simpler than I thought! Though I think that perhaps the cache itself also needs to become a WeakValueDictionary so that, say, if one has tt = t.tt and later does del tt, the cache does not keep tt alive. But probably better as a separate PR since that would not be a bug fix. EDIT: on second thought, not sure WeakValueDictionary would work for "scale", since I think part of the original rationale was that one could do jd1 = t.tt.jd1; jd2 = t.tt.jd2 and not have tt calculated twice. Anyway, that's orthogonal to this PR.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@mhvk mhvk added time Bug 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.3.x on-merge: backport to v5.3.x labels Oct 9, 2023
@mhvk mhvk added this to the v5.0.9 milestone Oct 9, 2023
@mhvk mhvk requested a review from taldcroft as a code owner October 9, 2023 11:40
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@mhvk mhvk mentioned this pull request Oct 9, 2023
Closed
@mhvk mhvk force-pushed the time-cache-coherence branch 3 times, most recently from 38fe2ea to 1eb8d00 Compare October 9, 2023 13:20
@mhvk
Copy link
Contributor Author

mhvk commented Oct 9, 2023 •
edited

Well, I wrote a bit too quickly about the simplicity - caches never are simple, I guess. But still the PR is not bad, it is just that some of the care one has to take is not entirely obvious. Anyway, now the tests pass (devdeps failures are unrelated).

mhvk
mhvk commented Oct 9, 2023
View reviewed changes
astropy/time/core.py
def __getstate__(self):
# For pickling, we remove the cache from what's pickled
state = (self.__dict__ if PYTHON_LT_3_11 else super().__getstate__()).copy()
state.pop("_id_cache", None)
Copy link
Contributor Author

@mhvk mhvk Oct 9, 2023 •
edited

Choose a reason for hiding this comment

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

This is necessary because WeakValueDictionary cannot be pickled (the similar change in TimeFormat removal of cache is just because there is no sense to pickle a cache generally).

@taldcroft
Copy link
Member

@mhvk - sorry, this got off of my radar. I'm guessing that it will need an update after #15231 is merged since the cache moved, but otherwise this looks generally good to me so we should get it in to 6.0.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 27, 2023

No worries, the rebase of this was easier than the other way around! While rebasing, I also parametrized the test to check masked times as well, since masked instances do not own their data directly. It all feels a bit fragile -- caching is hard!

taldcroft
taldcroft approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

As noted, caching is difficult. But as far as I can tell this looks good. 🤞

@mhvk mhvk merged commit 955e810 into astropy:main Oct 27, 2023
24 of 26 checks passed
@lumberbot-app
Copy link

lumberbot-app bot commented Oct 27, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v5.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 955e810650f1bf8cd159f71b54afe7ea6f43a77d
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #15453: Ensure cache coherence for views of time instances'
  1. Push to a named branch:
git push YOURFORK v5.0.x:auto-backport-of-pr-15453-on-v5.0.x
  1. Create a PR against branch v5.0.x, I would have named this PR:

"Backport PR #15453 on branch v5.0.x (Ensure cache coherence for views of time instances)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 27, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v5.3.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 955e810650f1bf8cd159f71b54afe7ea6f43a77d
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #15453: Ensure cache coherence for views of time instances'
  1. Push to a named branch:
git push YOURFORK v5.3.x:auto-backport-of-pr-15453-on-v5.3.x
  1. Create a PR against branch v5.3.x, I would have named this PR:

"Backport PR #15453 on branch v5.3.x (Ensure cache coherence for views of time instances)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 27, 2023

Yes, I think it is OK - mostly because we test this well!

Note that I can still backport, but it now needs to be manual given Masked -- do you feel this is worth it, or shall I just change the milestone?

@mhvk mhvk deleted the time-cache-coherence branch October 27, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taldcroft taldcroft taldcroft approved these changes

Assignees
No one assigned
Labels
Bug Still Needs Manual Backport time 💤 backport-v5.0.x on-merge: backport to v5.0.x 💤 backport-v5.3.x on-merge: backport to v5.3.x
Projects
None yet
Milestone
v5.0.9
Development

Successfully merging this pull request may close these issues.

Time cache can get out of sync
2 participants
@mhvk @taldcroft