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

Silence warning #647

Merged
merged 1 commit into from
Dec 21, 2024
Merged

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Dec 11, 2024

Recent developments in aiidalab-home of an external notebook to manage codes run into an issue with the AWB computational resources widgets when used without a default calcjob plugin. This PR silences a suspected false warning for now, to be properly investigated in a future PR.

Handling issue #648

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.47%. Comparing base (fd8c1b0) to head (d082f0a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #647   +/-   ##
=======================================
  Coverage   83.46%   83.47%           
=======================================
  Files          17       17           
  Lines        3556     3558    +2     
=======================================
+ Hits         2968     2970    +2     
  Misses        588      588           
Flag Coverage Δ
python-3.11 83.47% <100.00%> (+<0.01%) ⬆️
python-3.9 83.51% <100.00%> (+<0.01%) ⬆️

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.

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Could open an issue so that we don't forget to investigate it in the future?

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Just one question. Agreed about opening an issue and link to it in the TODO comment.

Also can you please expand the PR desription with how the problem manifests exaxtly? Is there a warning in the UI? Does the code crash?

@@ -1302,6 +1302,10 @@ def _observe_code_setup(self, _=None):
for key, value in self.code_setup.items():
if hasattr(self, key):
if key == "default_calc_job_plugin":
if "None" in value:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the type of value? Somewhat surprising if it would be list for tuple?

Copy link
Member Author

Choose a reason for hiding this comment

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

A string. It's an entry point. Like quantumespresso.pw. But if you don't associate a default calcjob plugin, when you select the Quantum ESPRESSO code from the code selector in the resource setup widget, the value somehow ends up being quantumespresso.None. So I check if the string None is in the entry point string. But yes, need to investigate why this happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay, that definitely needs an explanatory comment in the code, because "None" in value looks suuuper weird 😄

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Dec 13, 2024

Just one question. Agreed about opening an issue and link to it in the TODO comment.

Also can you please expand the PR desription with how the problem manifests exaxtly? Is there a warning in the UI? Does the code crash?

Referenced the issue, where the matter is described in detail. Added a reference in the TODO comment. @danielhollas let me know if I can merge this 🙏

@edan-bainglass
Copy link
Member Author

@danielhollas Wow! I'm at a loss as to how changing the comment now causes a test to fail 🤦🏻‍♂️

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Sure, go ahead! I am a bit concerned this hacky fix will bite us in the future unless we investigate it properly, but it's fine for now. :-)

@danielhollas Wow! I'm at a loss as to how changing the comment now causes a test to fail 🤦🏻‍♂️

Looks like a test flake, tests are passing after I rerun them. Could you open an issue about a flaky test_cod_widget, including the errors from the CI? That test is accessing an external database, we should mock that out.

Thanks!

@edan-bainglass
Copy link
Member Author

Sure, go ahead! I am a bit concerned this hacky fix will bite us in the future unless we investigate it properly, but it's fine for now. :-)

@danielhollas Wow! I'm at a loss as to how changing the comment now causes a test to fail 🤦🏻‍♂️

Looks like a test flake, tests are passing after I rerun them. Could you open an issue about a flaky test_cod_widget, including the errors from the CI? That test is accessing an external database, we should mock that out.

Thanks!

Indeed, very flaky, as I also re-ran the tests before making my previous comment, but they still failed 😢 Okay, opening an issue.

@edan-bainglass edan-bainglass merged commit 0542daa into aiidalab:master Dec 21, 2024
11 checks passed
unkcpz added a commit to edan-bainglass/aiidalab-widgets-base that referenced this pull request Jan 4, 2025
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.

3 participants