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

Add ArithmeticAdd CJ Node fixture without run or submit #6733

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

GeigerJ2
Copy link
Contributor

@GeigerJ2 GeigerJ2 commented Jan 28, 2025

This PR introduces a fixture to obtain an ArithmeticAdd CalcJobNode that doesn't require to actually run the Process. Instead, the incoming and outgoing links (and properties) are manually added (set). Not sure if this will be actually useful for testing in the future, but I'd probably put it under nice-to-have.

There are likely still a few details missing to get exactly the same Node, as one obtains when actually running the Process (which is typically done in the test suite via the builder/factory and inputs), and I haven't checked if suitable tests would also run through with this fixture. Though, at least for my dumping-related tests, the obtained node gives the same results. One could also do the same for the MultiplyAddWorkChain, if it turns out to be actually useful to be able to obtain CalcJobNodes and WorkChainNodes for testing that don't to rely on actually submitting to/running on a Computer.

The main motivation that led to adding the fixture of this PR was to be able to add tests to #6723, where I would like to construct a profile with some CalcJobNodes and WorkChainNodes, organized in groups, etc., for testing the profile mirror functionality. However, I'd like to have the setup of this profile's data structure to be class or session-scoped, as it takes some compute time to set up, and should not be repeated for every test method. However, the generate_calculation_node_add fixture I wanted to use is function-scoped and therefore pytest throws a ScopeMismatch exception... (and so are the fixtures it relies on, such as aiida_localhost)

Pinging @khsrali and @unkcpz, if you have comments <3

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.13%. Comparing base (d2fbf21) to head (3bec4e5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6733      +/-   ##
==========================================
+ Coverage   78.13%   78.13%   +0.01%     
==========================================
  Files         564      564              
  Lines       42526    42526              
==========================================
+ Hits        33222    33223       +1     
+ Misses       9304     9303       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz
Copy link
Member

unkcpz commented Feb 6, 2025

I think this is a nice thing to have, for the tests that we don't want to actually test the transport this is a good gain on performance.

@GeigerJ2
Copy link
Contributor Author

GeigerJ2 commented Feb 6, 2025

Note to self. There exists a similar fixture (factory) in aiida-qe:
https://github.com/aiidateam/aiida-quantumespresso/blob/c4d7c190456deca0c26e4fc36304e973fe14a2c2/tests/conftest.py#L208

Also pinging @edan-bainglass, as @unkcpz mentioned you were also looking for this in the past. I'll clean up the code here a bit, check how it's done in the QE fixture, and implement a version for the MultiplyAdd, as well.

Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

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

Thanks a lot @GeigerJ2, for pinging me.

I feel like if you start from this fixture it's already easier, generate_calcjob_node.
You can whatever files hierarchy or incoming links to it.
I'd use it this way, for example:

  node = generate_calcjob_node()
  node_workdir = computer_wdir / uuid[:2] / uuid[2:4] / uuid[4:]
  pathlib.Path(node_workdir).mkdir(parents=True)
  node.set_remote_workdir(str(node_workdir))
  create_file_hierarchy(file_hierarchy, node_workdir)
  node.store()
  ## in case you need to add links:
  node.base.links.add_incoming(another_node, link_type=LinkType.CREATE, link_label='another_node')

Let me know if this works.

@GeigerJ2
Copy link
Contributor Author

Hi @khsrali, that's a good idea. Initially, I didn't use the generate_calcjob_node fixture, because it is also function-scoped. Though, as of now, I just accept that and disregard fixture scopes, so I changed the code to use your proposed approach and avoid code repetition 👍🏽

To ensure the resulting CalcJobNodes are equivalent when being run or manually constructed, I checked all instance attributes by adding a temporary test function to test_conftest.py (thanks ChatGPT) and debugging:

def test_generate_calculation_node_add(generate_calculation_node_add, run_calculation_node_add):

    from rich import print
    from rich.pretty import Pretty

    def get_instance_attributes(obj):
        """Retrieve and display all attributes of an object, and optionally save to a JSON file.

        - Filters out methods and special (`__dunder__`) attributes.
        - Pretty-prints the attributes using `rich`.

        :param obj: The object whose attributes should be retrieved.
        """
        attributes = {
            attr: getattr(obj, attr)
            for attr in dir(obj)
            if not attr.startswith("__") and not callable(getattr(obj, attr))
        }

        # Pretty-print the attributes
        print(Pretty(attributes, expand_all=True))

    nm = generate_calculation_node_add()
    nr = run_calculation_node_add()

    breakpoint()

Doing a diff on the resulting outputs shows that only PK, UUID, datetime, and memory locations are different (all of which are expected ofc):

image

I'll now see if the tests still pass if one uses the new fixture (where applicable), and then I think this should be ready for review.

@GeigerJ2 GeigerJ2 force-pushed the tests/arithmetic-add-fixture-no-run branch from 17c4fad to 42481b9 Compare February 12, 2025 10:47
@GeigerJ2 GeigerJ2 marked this pull request as ready for review February 12, 2025 11:05
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