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 CI pipeline #28

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

Conversation

steinmig
Copy link

  • Moves existing tests to separate folder
  • Convert some existing tutorials into regression tests
  • CI pipeline with only default installation and pytest execution for now

Copy link
Contributor

@ajhoffman1229 ajhoffman1229 left a comment

Choose a reason for hiding this comment

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

I think there might be a few changes worth making before merging this branch. Namely:

  1. reverting the use of the _ax imports (these need to be integrated with other files and removed, so we don't want any new imports from them if we can avoid it)
  2. cleaning up tests so that they're distinct from tutorials (removing excess comments, etc.)

Some of the training can also take quite a long time to run as it exists in the tutorials right now... Do you think there we should take precautions to ensure the CI/CD pipeline doesn't take too long to run?

Comment on lines +18 to +19
from nff.md.utils_ax import mol_dot, mol_norm, ZhuNakamuraLogger, atoms_to_nxyz
from nff.md.nvt_ax import NoseHoover, NoseHooverChain
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that we want to make this change... I believe the _ax files are modified versions from a former student (Simon Axelrod) and they may be out-of-date.

This duplicate code is one of many issues in NFF... I remember at one point that Simon suggested doing away with those versions entirely or integrating them with the main files, but they work for chemistry I'm less familiar with (excited states) and Simon didn't have time to migrate and update the code himself. I think I will open a specific issue and see if I can recruit him to check changes that we make.

Copy link
Author

Choose a reason for hiding this comment

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

I suspected something like we have already discussed concerning deprecated code might be going on with these _ax files. I anyway changed the import here, because

  1. some of the imported objects do not exist in the files, so the current import statement is broken
  2. it is just a test, so we do not change actual production runs.

If this test ever makes any issues, because the _ax files make some problems or we remove them, I would say we can look into removing this test or adapting it to the current code base?

Comment on lines +8 to +9
import pytest

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be a bit out of touch, but is there an advantage to using pytest over unittest? I normally just use unittest. While I know that pytest serves a similar function and I've heard of it, I'm unfamiliar with formatting and syntax differences between the two and which integrates better with GitHub CI/CD.

Copy link
Author

Choose a reason for hiding this comment

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

I never looked into it too much, because it "just works" and pytest is the most commonly used testing system, but to my understanding the advantages are:

  1. Execution is quite simple, pytest follows an easy rule which functions in which files to execute and executes just these functions, so there is no extra code in the test files necessary
  2. Pytest integrates very well with easily available coverage reports
  3. In our case we have the CPU/GPU problem with most of the code base and pytest offers a not super easy, but relatively easy way to adjust variables in tests based on user settings when executing pytest. So in this PR I also add that you can do pytest --device cpu or pytest --device cuda to execute the tests either with CPU or GPU. CPU is the default, because it is easier on the CI. So far this is not documented, but I would focus on documentation / readme after linting the code base, but definitely worth putting on a list

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file a modified version of the tutorial for training a potential for molecules with excited states? If so, maybe we can add a file docstring at the top that describes its origin.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this file and test_training are regression test I copied from the tutorials. Adding a remark and removing some of the comments sounds good

Copy link
Author

Choose a reason for hiding this comment

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

I mean ideally one would want to simply test all of the tutorials, so we would also immediately know once they are out of date, but

  1. I am not too familiar with notebooks in a CI context
  2. Most of the current tutorials do not work out of the box at the moment -> also worth putting on a list

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the comments here also look like they are from a notebook tutorial... I can try to make some modifications and updates to these files on this branch before merging to make sure that the tests are clear and distinct from the tutorials.

@steinmig
Copy link
Author

1. reverting the use of the `_ax` imports (these need to be integrated with other files and removed, so we don't want any new imports from them if we can avoid it)

see thread above

2. cleaning up tests so that they're distinct from tutorials (removing excess comments, etc.)

yes makes sense

Some of the training can also take quite a long time to run as it exists in the tutorials right now... Do you think there we should take precautions to ensure the CI/CD pipeline doesn't take too long to run?

yes, I reduced the epochs and increased the expected MAE from the tutorials to save some time. The excited states still take ~20 minutes, which is why I have disabled the test for now. The general potential training only takes a few seconds. You can see the CI execution time under Actions

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.

2 participants