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.

Sign up for GitHub

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

Jump to bottom

Fix/988/pdosworkchain #993

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Fix/988/pdosworkchain #993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
454367c
mod fermi_energy in pdosworkchain
AndresOrtegaGuerrero Dec 12, 2023
ebfef07
remove nscf_fermi_energy_up/down from context
AndresOrtegaGuerrero Dec 12, 2023
68bd925
Add test
mbercx Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/aiida_quantumespresso/workflows/pdos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,12 @@ def inspect_nscf(self):
self.ctx.nscf_emin = workchain.outputs.output_band.get_array('bands').min()
self.ctx.nscf_emax = workchain.outputs.output_band.get_array('bands').max()
self.ctx.nscf_parent_folder = workchain.outputs.remote_folder
self.ctx.nscf_fermi = workchain.outputs.output_parameters.dict.fermi_energy
if 'fermi_energy' in workchain.outputs.output_parameters.dict:
self.ctx.nscf_fermi = workchain.outputs.output_parameters.dict.fermi_energy
else:
fermi_energy_up = workchain.outputs.output_parameters.dict.fermi_energy_up
fermi_energy_down = workchain.outputs.output_parameters.dict.fermi_energy_down
self.ctx.nscf_fermi = max(fermi_energy_down, fermi_energy_up)

def _generate_dos_inputs(self):
"""Run DOS calculation, to generate total Densities of State."""
Expand Down
13 changes: 8 additions & 5 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# -*- coding: utf-8 -*-
# pylint: disable=redefined-outer-name,too-many-statements
# pylint: disable=redefined-outer-name,too-many-statements,unsubscriptable-object
"""Initialise a text database and profile for pytest."""
from collections.abc import Mapping
import io
Expand Down Expand Up @@ -51,14 +51,17 @@ def fixture_code(fixture_localhost):
"""Return an ``InstalledCode`` instance configured to run calculations of given entry point on localhost."""

def _fixture_code(entry_point_name):
from aiida.common import exceptions
from aiida.orm import InstalledCode, load_code
from aiida.orm import InstalledCode, QueryBuilder

label = f'test.{entry_point_name}'

query = QueryBuilder().append(
InstalledCode,
filters={'label': label},
)
try:
return load_code(label=label)
except exceptions.NotExistent:
return query.first()[0]
except TypeError:
Comment on lines +58 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? @mbercx I don't understand this nor do I think it is correct. I would revert this

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% what was happening under the pytest hood, but when using pytest.mark.parametrize to test the two possible NSCF output parameters, it seems to somehow have created two instances of the quantumespresso.pw code, and the second test fails with:

aiida.common.exceptions.MultipleObjectsError: multiple Code entries found with LABEL<test.quantumespresso.pw>

My solution is a bit of a late-night hack, I'll admit. ^^

Looking into this further, it actually seems that VSCode running the test in parallel causes the issue. After reverting reverting the change here, the tests still pass fine. Running pytest manually locally also works without issue.

Happy to open a PR to revert, but maybe there is also a more elegant solution to have the tests run properly in parallel?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the tests are not designed to be run in parallel, especially not if they use the same storage backend. Even if you "fix" this case, there is bound to be a shitload of other problems lying in wait when you have multiple tests run against the same database. So I really think you just shouldn't run them in parallel.

Copy link
Member

Choose a reason for hiding this comment

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

Opened #995 to revert.

return InstalledCode(
label=label,
computer=fixture_localhost,
Expand Down
18 changes: 16 additions & 2 deletions tests/workflows/test_pdos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# -*- coding: utf-8 -*-
# pylint: disable=unused-argument
# pylint: disable=unused-argument,too-many-statements
"""Tests for the `PdosWorkChain` class."""
from __future__ import absolute_import

Expand All @@ -8,6 +8,7 @@
from aiida.engine.utils import instantiate_process
from aiida.manage.manager import get_manager
from plumpy import ProcessState
import pytest

from aiida_quantumespresso.calculations.helpers import pw_input_helper

Expand All @@ -26,6 +27,17 @@ def instantiate_process_cls(process_cls, inputs):
return instantiate_process(runner, process_cls, **inputs)


@pytest.mark.parametrize(
'nscf_output_parameters', [
{
'fermi_energy': 6.9
},
{
'fermi_energy_down': 5.9,
'fermi_energy_up': 6.9
},
]
)
def test_default(
generate_workchain_pdos,
generate_workchain_pw,
Expand All @@ -35,6 +47,7 @@ def test_default(
generate_calc_job_node,
fixture_sandbox,
generate_bands_data,
nscf_output_parameters,
):
"""Test instantiating the WorkChain, then mock its process, by calling methods in the ``spec.outline``."""

Expand Down Expand Up @@ -76,7 +89,7 @@ def test_default(
remote.store()
remote.base.links.add_incoming(mock_wknode, link_type=LinkType.RETURN, link_label='remote_folder')

result = orm.Dict({'fermi_energy': 6.9029595890428})
result = orm.Dict(nscf_output_parameters)
result.store()
result.base.links.add_incoming(mock_wknode, link_type=LinkType.RETURN, link_label='output_parameters')

Expand All @@ -87,6 +100,7 @@ def test_default(
wkchain.ctx.workchain_nscf = mock_wknode

assert wkchain.inspect_nscf() is None
assert 'nscf_fermi' in wkchain.ctx

# mock run dos and projwfc, and check that their inputs are acceptable
dos_inputs, projwfc_inputs = wkchain.run_pdos_parallel()
Expand Down
Loading