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

experiments: add state cleanup for deleted experiments #2611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fsagbuya
Copy link
Contributor

ARTIQ Pull Request

Description of Changes

Add cleanup_state method to handle stale repository experiments and prevent state accumulation. Ensures proper cleanup of submission scheduling, options, arguments and UI states when repository experiments are deleted.

NOTE: Requires merging #2590 first.

Related Issue

Closes #2576

Type of Changes

Type
✨ New feature

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

  • Use correct spelling and grammar.
  • Update RELEASE_NOTES.rst if there are noteworthy changes, especially if there are changes to existing APIs.
  • Close/update issues.
  • Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

  • Test your changes or have someone test them. Mention what was tested and how.
  • Add and check docstrings and comments

Git Logistics

  • Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
  • Write short & meaningful commit messages. Review each commit for messages (git show). Format:
    topic: description. < 50 characters total.
    
    Longer description. < 70 characters per line
    

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

Comment on lines +717 to +718
if expurl in self.open_experiments:
self.open_experiments[expurl].close()
Copy link
Member

Choose a reason for hiding this comment

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

Why would you delete the state of an experiment if it's open?

Copy link
Member

Choose a reason for hiding this comment

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

And this code is likely never called? See:

    def restore_state(self, state):
        if self.open_experiments:
            raise NotImplementedError
   ...

@sbourdeauducq
Copy link
Member

sbourdeauducq commented Dec 11, 2024

The logic of this seems broken at several levels. Experiments open outside the repos are never cleared. The fact that the cleanup only happens in restore_state doesn't make sense.

Limiting the size of the relevant dictionaries by removing out old entries when a certain size is reached (see https://www.google.com/search?q=python+lru+dict) sounds simpler and more robust.

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.

user-defined title bar and background color for experiment windows
2 participants