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

Updating test suite to yt-4.x and concurrent testing #190

Merged
merged 23 commits into from
Jul 14, 2022

Conversation

jwise77
Copy link
Contributor

@jwise77 jwise77 commented Jul 5, 2022

This PR updates all testing infrastructure to be compatible with yt-4.x, including

  • fields as tuples
  • removing calls to the halo mass function
  • referring to yt_astro_analysis for HaloCatalog calls
  • removing yt.mods calls
  • replacing na with np (!)

I created a new gold standard (v15) because the old gold-standard cannot be compared to the new runs without some changes to the testing environment (e.g. different packages and dependencies). I manually confirmed on my local machine that the latest PR (#132) passed the push suite before making the new gold standard.

Another big change was using process pools in the multiprocessing python module to run the test simulations concurrently. This saves about 10 minutes in the push suite simulations. I couldn't get the testing to run in parallel without putting more effort into it. This could be addressed in a future PR. I increased the CircleCI resource from medium to large since we can run tests in parallel now, and the large nodes are still free to us.

Note: There are a lot of duplicate changes in common code in various files, so the review should be easier than what the diff size says.

@jwise77 jwise77 force-pushed the fix-test-suite branch 2 times, most recently from 78f5ab4 to a6707c3 Compare July 13, 2022 19:10
Copy link
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.

A heroic effort, John! Thanks for doing this. This looks good to me.

@brittonsmith
Copy link
Contributor

Now that the yt infrastructure we rely on is going away, we may want to think about moving to something like what we've setup for Enzo-E. That system can be parallelized automatically using circleci.

Copy link
Contributor

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Thanks for the fixing this. Looks good to me. I only have a couple of non-blocking clarification questions.

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
Copy link
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 -- just a few duplicated imports. Thanks for doing all of this!

@gregbryan
Copy link
Contributor

Now that the yt infrastructure we rely on is going away, we may want to think about moving to something like what we've setup for Enzo-E. That system can be parallelized automatically using circleci.

Britton -- Can you be more specific about what exactly is going away?

@jwise77
Copy link
Contributor Author

jwise77 commented Jul 14, 2022

Now that the yt infrastructure we rely on is going away, we may want to think about moving to something like what we've setup for Enzo-E. That system can be parallelized automatically using circleci.

That's good to know about the answer testing infrastructure going away. I've made issue #192 to remind us about it.

jwise77 and others added 5 commits July 14, 2022 10:27
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
@jwise77
Copy link
Contributor Author

jwise77 commented Jul 14, 2022

Looks good to -- just a few duplicated imports. Thanks for doing all of this!

You're welcome! I've committed your suggestions. Those duplicates were left over from my find & replace!

@brittonsmith
Copy link
Contributor

Now that the yt infrastructure we rely on is going away, we may want to think about moving to something like what we've setup for Enzo-E. That system can be parallelized automatically using circleci.

Britton -- Can you be more specific about what exactly is going away?

The yt project is transitioning its answer testing from nose to pytest. The AnswerTesting class we import in test_runner.py from yt will be quite different from how it is now. I'm not sure what compatibility the new pytest framework will have with our current setup. I will investigate.

@gregbryan
Copy link
Contributor

This looks ready to go -- thanks @jwise77 for fixing testing; now we can address many outstanding pull requests. I'm merging.

@gregbryan gregbryan merged commit a01a390 into enzo-project:main Jul 14, 2022
@jwise77 jwise77 deleted the fix-test-suite branch July 14, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants