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

Refactor the listener to allow for being centralized #394

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JamesWrigley
Copy link
Member

@JamesWrigley JamesWrigley commented Feb 13, 2025

Important changes:

  • The listener now supports triggering jobs for either all proposals or what proposals are already in its database.
  • In addition to the 'official' databases under usr/Shared/amore, it's
    possible to add unofficial databases at any location.
  • The GUI now only initializes the database and assumes a listener is already
    running.

I've tried to keep the commits atomic so I would suggest reviewing them one-by-one. @RobertRosca, @CammilleCC, I believe this is ready to be tested.

@JamesWrigley JamesWrigley self-assigned this Feb 13, 2025
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 76.79558% with 42 lines in your changes missing coverage. Please review.

Project coverage is 76.87%. Comparing base (df15918) to head (b5a7ba9).
Report is 21 commits behind head on master.

Files with missing lines Patch % Lines
damnit/cli.py 60.52% 15 Missing ⚠️
damnit/backend/test_listener.py 26.66% 11 Missing ⚠️
damnit/backend/listener.py 88.05% 8 Missing ⚠️
damnit/backend/db.py 87.50% 4 Missing ⚠️
damnit/api.py 66.66% 2 Missing ⚠️
damnit/gui/main_window.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage   75.70%   76.87%   +1.16%     
==========================================
  Files          35       35              
  Lines        5932     6123     +191     
==========================================
+ Hits         4491     4707     +216     
+ Misses       1441     1416      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This isn't particularly clean, but the delay is only 0.1s so it should be
acceptable.
Two changes:
- Access the environment variable override in the function so we can set it at
  runtime.
- Take in an integer proposal number and apply formatting internally.
This allows them to specify which DAMNIT environment to use for processing
runs, independently of what environment the processing is triggered from.
Important changes:
- The listener now supports triggering jobs for either all proposals or a single
  one.
- In addition to the 'official' databases under `usr/Shared/amore`, it's
  possible to add unofficial databases at any location.
- The GUI now only initializes the database and assumes a listener is already
  running.
For some reason it was still running afterwards, which would cause segfaults.
Amusingly, this will otherwise semi-randomly cause segfaults
when... something... gets garbage collected before it should be.
@JamesWrigley
Copy link
Member Author

I extended the tests and added some docs here: https://damnit--394.org.readthedocs.build/en/394/internals/#the-listener

@JamesWrigley
Copy link
Member Author

Hmph, just realized that the limit on the number of concurrent jobs should be per-proposal rather than global. Will fix that.

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.

1 participant