Skip to content

Improve ergonomics of pytest machinery#318

Merged
mabruzzo merged 10 commits intoenzo-project:mainfrom
mabruzzo:pytest-argparse
Aug 1, 2023
Merged

Improve ergonomics of pytest machinery#318
mabruzzo merged 10 commits intoenzo-project:mainfrom
mabruzzo:pytest-argparse

Conversation

@mabruzzo
Copy link
Copy Markdown
Contributor

@mabruzzo mabruzzo commented Jun 12, 2023

This PR introduces a number of small changes that should make the pytest machinery easier to use, particularly when you are switching between multiple branches.

These changes include:

  1. Introducing the ability to configure machinery by passing command-line arguments in addition to setting environment variables. Below I list new command-line arguments with their env variable counterparts (when both counterparts are set, the command-line flags have priority):
    • --charm and CHARM_PATH
    • --grackle-input-data-dir and GRACKLE_INPUT_DATA_DIR
    • --local-dir and TEST_RESULTS_DIR (the command line flag borrows the name that yt uses)
    • --answer-store and GENERATE_TEST_RESULTS (the command line flag borrows the name that yt uses)
  2. I introduced the new flag called --build-dir which you can use to specify the directory where the target Enzo-E binary was built. If --charm and CHARM_PATH are not specified, then the test program will infer the appropriate location of the charmrun executable based on the contents of CMakeCache.txt.
  3. I added the ability for the testing infrastructure to directly query what the compiled-precision is (obviating the need for the USE_DOUBLE environment variable), and querying whether the executable was built with Grackle (tests no longer fail problem if GRACKLE_INPUT_DATA_DIR is set and enzo-e was compiled without Grackle - those tests are now skipped).

As an aside, I completely removed the ENZO_PATH variable as it's no longer necessary (and I'm pretty sure that I'm the only person to use it). Everything else should be fully backwards-compatible.


With these changes, it's much easier to invoke the pytest machinery while moving between different branches that are built in different build directories (and may depend on different versions of charm++).

For example, if I have the build-directories

  • build-intel-impi
  • build-gcc-openmpi

I can invoke pytest from the root directory with:

  • pytest test/answer_test --build-dir ./build-intel-impi --local-dir ~/enzoe-gs-intel
  • pytest test/answer_test --build-dir ./build-gcc-openmpi --local-dir ~/enzoe-gs-gcc

While this is verbose, I would have previously needed to be sure to update the ENZO_PATH, CHARM_PATH, and TEST_RESULTS_DIR environment variables. I might also need to adjust GRACKLE_INPUT_DATA_DIR or USE_DOUBLE if one of these builds was compiled with/without grackle or with a different precision.

Additional improvements could potentially be made. For example, it may be sensible for pytest to assume that it is invoked from a build-directory when it is invoked without --build-dir (unless it is invoked from the root directory). But, that's a topic for another day.


In the course of this PR, I refactored the functionality for executing an enzo-e simulation into a class called EnzoEDriver and moved it into a new test_utils sub-directory.

  • Currently there are a bunch of wrappers for executing enzo-e floating around the repository. The idea was to provide a single centralized implementation that other python code can call into. While most of these other wrappers occur in test code that should eventually be integrated into the pytest, I expect it may still be useful to have this centralized wrapper.

  • In any case, it was important to factor out the enzo-e wrapper from the rest of the answer-testing machinery. This will be important for new checkpoint-restart tests that I'm adding to the pytest machinery, which are nearly ready for review.

  • Along the way, I also added the ability to allow the enzo-e wrapper to easily modify the parameters used in the simulation. This was inspired by what athena++ does and greatly simplifies the code for forthcoming checkpoint-restart tests and convergence tests. (Note: modifying the parameters currently happens outside of the enzo-e binary, by creating a new parameter file)

EDIT: Reviewing this PR is important because it is a dependency of PR #324, which should be a high priority

@mabruzzo mabruzzo added the testing: answer-tests Changes to the answer-testing machinery label Jul 3, 2023
Copy link
Copy Markdown
Contributor

@gregbryan gregbryan left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -86,18 +124,12 @@ First, check out the highest numbered gold standard tag and compile ``enzo-e``.
$ git checkout gold-standard-1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this is not changed in this PR, but aren't we on gold-standard-004?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I have updated this accordingly (and I've added a note commenting that readers will need to update the number in the future)

else:
enzoe_path = os.path.join(_root_dir, 'build/bin/enzo-e')
if not os.path.isfile(enzoe_path):
raise RuntimeError(f"enzo-e isn't at {enzoe_path}. If the it is "
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise RuntimeError(f"enzo-e isn't at {enzoe_path}. If the it is "
raise RuntimeError(f"enzo-e isn't at {enzoe_path}. If it is "

Copy link
Copy Markdown
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Yeah, this looks good. Just a couple typos I caught, but happy for this to be merged.

This is useful for testing problems with no analytical solution or generally
verifying that results from commonly run simulations don't drift.

It is also useful for testing property of simulation runs beyond the exit-time/cycle of the simulation.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sentence is a bit awkward. I'm not sure what it's trying to say.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I'm not sure what I was saying here either... (I was probably referencing the fact that most tests in the ctest framework just check the final simulation time/cycle)

I replaced the whole paragraph, which was originally written as

It is also useful for testing property of simulation runs beyond the
exit-time/cycle of the simulation.  (While such tests do exist in the
ctest-framework, they often involve more boiler-plate code).

with the following snippet:

It is also useful in for testing problems that do have analytic
solutions (the answer test might quantify how close a simulation
result is to the analytic expected solution).  While such tests do
exist in the ctest-framework, they often involve more boiler-plate
code.

mabruzzo and others added 3 commits July 28, 2023 11:36
Co-authored-by: Britton Smith <brittonsmith@gmail.com>
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
@mabruzzo
Copy link
Copy Markdown
Contributor Author

@brittonsmith and @gregbryan -- let me know if my changes addressed your concerns

Copy link
Copy Markdown
Contributor

@gregbryan gregbryan left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Copy Markdown
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

All good here.

@mabruzzo mabruzzo merged commit 9bfe665 into enzo-project:main Aug 1, 2023
@mabruzzo mabruzzo deleted the pytest-argparse branch August 1, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing: answer-tests Changes to the answer-testing machinery

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants