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

Replace check of empty string in unfilled template widgets to None #650

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

edan-bainglass
Copy link
Member

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

This PR corrects a guard for unfilled template variables which checks if the value of the corresponding widget is an empty string. Empty widgets may also be represented by None, especially dropdown selectors. This PR adds None to the check by comparing the widget's value to both - replaces == "" with in ("", None).

Resolves #648
Resolves #651

@superstar54 @danielhollas have a look 🙏
@unkcpz tagging you due to git blame (suggests you implemented these sections)

Copy link

codecov bot commented Dec 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.45%. Comparing base (7a93035) to head (90e4ba5).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
- Coverage   83.46%   83.45%   -0.01%     
==========================================
  Files          17       17              
  Lines        3568     3566       -2     
==========================================
- Hits         2978     2976       -2     
  Misses        590      590              
Flag Coverage Δ
python-3.11 83.45% <100.00%> (-0.01%) ⬇️
python-3.9 83.49% <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.

@edan-bainglass edan-bainglass marked this pull request as ready for review December 22, 2024 07:23
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.

LGTM but I am not familiar with the template code at all so this better be reviewed by somebody that does :-)

@danielhollas
Copy link
Contributor

Also thanks for following up on this @edan-bainglass! 🤟

@edan-bainglass
Copy link
Member Author

LGTM but I am not familiar with the template code at all so this better be reviewed by somebody that does :-)

100% why I tagged @unkcpz 😅

@edan-bainglass
Copy link
Member Author

@unkcpz can you have a look at this?

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @edan-bainglass, I think it is a correct change, None was what I didn't considered when initialize the widget value. This will for sure directly solve #648

Not sure it is easy to understand logic of testing, but when I created this widget, I tried to catch most of conditions with tests in

def test_template_variables_widget():
"""Test template_variables_widget."""
w = computational_resources.TemplateVariablesWidget()
w.templates = {
"label": "{{ label }}",
"hostname": "daint.cscs.ch",
"description": "Piz Daint supercomputer at CSCS Lugano, Switzerland, multicore partition.",
"transport": "core.ssh",
"scheduler": "core.slurm",
"work_dir": "/scratch/snx3000/{username}/aiida_run/",
"shebang": "#!/bin/bash",
"mpirun_command": "srun -n {tot_num_mpiprocs}",
"mpiprocs_per_machine": 36,
"prepend_text": "#SBATCH --partition={{ slurm_partition }}\n#SBATCH --account={{ slurm_account }}\n#SBATCH --constraint=mc\n#SBATCH --cpus-per-task=1\n\nexport OMP_NUM_THREADS=$SLURM_CPUS_PER_TASK\nsource $MODULESHOME/init/bash\nulimit -s unlimited",
"metadata": {
"slurm_partition": {
"type": "text",
"key_display": "Slurm partition",
},
},
}
# Fill the template variables
for key, value in w._template_variables.items():
if key == "label":
sub_widget = value.widget
sub_widget.value = "daint-mc-test"
# check the filled value is updated in the filled template
assert w.filled_templates["label"] == "daint-mc-test"
# Fill two template variables in one template line
for key, value in w._template_variables.items():
if key == "slurm_partition":
sub_widget = value.widget
sub_widget.value = "normal-test"
elif key == "slurm_account":
sub_widget = value.widget
sub_widget.value = "newuser"
# check the filled value is updated in the filled template
assert (
w.filled_templates["prepend_text"]
== "#SBATCH --partition=normal-test\n#SBATCH --account=newuser\n#SBATCH --constraint=mc\n#SBATCH --cpus-per-task=1\n\nexport OMP_NUM_THREADS=$SLURM_CPUS_PER_TASK\nsource $MODULESHOME/init/bash\nulimit -s unlimited"
)
# Test the filled template is updated when the filled value is updated.
for key, value in w._template_variables.items():
if key == "slurm_partition":
sub_widget = value.widget
sub_widget.value = "debug"
assert "debug" in w.filled_templates["prepend_text"]
. Would you mind to give it a look to add a test for this None situation? I guess you can add #648 example as test and use the change of this PR to test it.

@edan-bainglass edan-bainglass merged commit 1b245c0 into aiidalab:master Jan 5, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants