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

Add missing features in resource settings tabs #946

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

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Nov 26, 2024

Based on #945

This PR looks to clean up the recently implemented plugin-based tab system in the resources/submission step (#939).
The idea is to leverage the newly developed abstraction in #945 to encapsulate the behavior of ANY resource settings panel, leaving the global resource settings panel to override based on its unique added features.

A few added items addressed in this PR:

  1. PW code parallelization widget syncing between tabs
  2. Support for syncing newly setup codes
  3. Warning notification for overridden plugin resources
  4. Syncing back with global resources on un-override

Remaining issues:

The new approach somehow causes three tests, which pass in isolation, to fail when running the full suite.

FAILED tests/test_codes.py::test_check_submission_blockers - assert 0 == 1
 +  where 0 = len([])
 +    where [] = <aiidalab_qe.app.submission.model.SubmissionStepModel object at 0x7fa29d55edc0>.internal_submission_blockers
FAILED tests/test_codes.py::test_qeapp_computational_resources_widget - AssertionError: assert 'none' == 'block'
  - block
  + none
FAILED tests/test_submit_qe_workchain.py::test_create_builder_default - AssertionError: FILES DIFFER:

The last one I suspect may simply require a regeneration of the stored references.

But the first two, I suspect here it is a more serious issue that ONLY shows up in testing. I think the issue is that in testing, we generate more than a single instance of certain classes that have dict and/or list caches. We define these as class variables, which is "fine" for the app, but raises issues when various tests create instances of the class. A quick solution (if indeed this is the issue) is to define these as instance variables. However, this is not straight forward for the codes class variable shared by instances of ResourceSettingsModels.

There is no guarantee that this is the issue. Just a suspicion. Will test further as soon as I am not sick 🤒

@superstar54 good if you take a look, see if I missed something obvious. We can chat when I'm better.

Update

It was indeed the way we use collections as class variables. Tests passing now.

@edan-bainglass edan-bainglass marked this pull request as ready for review November 28, 2024 17:44
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 92.42424% with 15 lines in your changes missing coverage. Please review.

Project coverage is 68.48%. Comparing base (92b8299) to head (888d042).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/aiidalab_qe/common/panel.py 84.78% 7 Missing ⚠️
...iidalab_qe/app/submission/global_settings/model.py 91.83% 4 Missing ⚠️
src/aiidalab_qe/common/code/model.py 89.47% 2 Missing ⚠️
src/aiidalab_qe/app/submission/__init__.py 95.00% 1 Missing ⚠️
...dalab_qe/app/submission/global_settings/setting.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
+ Coverage   67.85%   68.48%   +0.63%     
==========================================
  Files         110      110              
  Lines        6206     6194      -12     
==========================================
+ Hits         4211     4242      +31     
+ Misses       1995     1952      -43     
Flag Coverage Δ
python-3.11 68.48% <92.42%> (+0.63%) ⬆️
python-3.9 68.49% <92.42%> (+0.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@edan-bainglass
Copy link
Member Author

@superstar54 ready for review 🙏

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