-
Notifications
You must be signed in to change notification settings - Fork 17
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
Template resources setting #511
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 79.92% 82.79% +2.87%
==========================================
Files 27 27
Lines 3815 4313 +498
==========================================
+ Hits 3049 3571 +522
+ Misses 766 742 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Thanks @danielhollas, I am thinking about it as well. I rebase it. |
55dff60
to
9e0deff
Compare
d2678fd
to
32a10e0
Compare
c8f8f00
to
7032f54
Compare
Hi @superstar54, @yakutovicha, this is ready for review, let me know how you want me to rebase it so you can review it more easily. It is currently every task has its own commit. I can rebase it to:
|
Hi @unkcpz thanks for the work. the UI looks more organized than before. I did a test using When I checked the code, I found there was a |
In the |
Good point! This is read from the template, it is the people who upload the template can set the name for this see https://aiidateam.github.io/aiida-resource-registry/database.json, I change it to "Computer Label" |
8eb7161
to
f75c79d
Compare
Hi @superstar54, could you try again? I find a bug in my code and it is now fixed see f75c79d |
The error still exists. And there is a new error now.
|
For the new error, it is because I move the |
ca86c2f
to
8ae9ec1
Compare
As discussed with @yakutovicha on Zoom, there are the following things need to be polished:
|
8ae9ec1
to
6f72d78
Compare
Hi @yakutovicha, I made all the changes as we discussed. You are right that for the weird behavior where the template placeholder not update, the codes are already there. Problem was when the default value does not exist for a template string, the For using the |
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.
@unkcpz thanks.
Unfortunately, two things that we discussed do not work:
- Template values are updated only when all fields are present.
- When I erased the value from the "Slurm account", it remained the same and did not return to
{{ slurm_account }}
Looking at the code I found several issues too, let's try to fix them.
As discussed with @unkcpz separately, I will do the remaining changes. |
…rceSetupBaseWidget
- pre-fill with default and use only one function to do it.
713c8f4
to
18e23f0
Compare
Hi @yakutovicha, I fix all the tests and try my best to clear the flow of the template class. Please have a "final" (:crossed_fingers:) look. |
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.
Congrats @unkcpz, this is a cool feature! 👏 |
Supersede #483, fixes #482
also fixes aiidalab/aiidalab-qe#240
XXX
001XXX
label 002 is cleaned.Feedbacks from @giovannipizzi after demo in the aiidalab meeting, can add in the template a section "meta" for information like
More tasks after @superstar54's test on CSCS machine.
More tasks after discuss with @yakutovicha.
username
now can be template for thecomputer_configure
. I think this is a very good design decision, we were not able to put username which is a mandatory field forverdi computer configure
since we don't want the code/resoruce registry contain specific information. Now thanks to the flexible template mechanism this information can be asked without specify the final value in the registry setup file.Notes for myself:
Before merge, instead of relying on the remoteaiida-resource-registry
database, mock one as fixture locally but validate it with the remote schema, this give us more flexibility to and robustness to run unit test.metadata.template_variables
?