-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix resource setup widget bugs #661
Fix resource setup widget bugs #661
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #661 +/- ##
==========================================
- Coverage 83.45% 83.39% -0.06%
==========================================
Files 17 17
Lines 3566 3578 +12
==========================================
+ Hits 2976 2984 +8
- Misses 590 594 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @edan-bainglass loots all good to me. Let's wait till tomorrow that we can rerun the tests.
@@ -1229,7 +1236,7 @@ def on_setup_code(self, _=None): | |||
qb = orm.QueryBuilder() | |||
qb.append(orm.Computer, filters={"uuid": computer.uuid}, tag="computer") | |||
qb.append( | |||
orm.AbstractCode, | |||
orm.Code, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed? orm.Code is deprecated so would be nice to avoid if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, Code worked where AbstractCode did not. But I think maybe InstalledCode should also work. Question is, do we support the other Code flavors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, AbstractCode does not work, allowing codes of existing full labels to go through!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why doesn't it work though? Is this a limitation of QueryBuilder? Or a bug in aiida-core?
In your testing, are the preexisting codes of Code
type or InstalledCode
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect they are all InstalledCode instances, but not sure. Will doubt check in the morning. It could be a QB issue. In any case, if Code works and covers all use cases and no resolution is found by tomorrow, I'll proceed with it for now, as this PR is holding up the release of the QE app. We can always revisit when we know more. Unless you think the use of Code is present breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, seems like a QB limitation. When using AbstractCode
, the node_type
field of the QB dictionary is {'like': 'data.code.abstract.%'}
, whereas for Code
, it is {'like': 'data.core.code.%'}
.
All my instances are of InstalledCode
, which have a node_type
of data.core.code.installed.InstalledCode
. I don't believe 'data.code.abstract'
is actually a node_type
of any of the Code
flavors. All code entry points are data.core.code...
.
For now (due to time constraints), I will proceed with Code
. Will open an issue+PR in aiida-core to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aiidateam/aiida-core#6687 for reference
This PR resolves the following bugs in code creation: