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

Per configuration pytest #261

Merged

Conversation

Felixrccs
Copy link
Contributor

First trial, lets see if this works.

@Felixrccs
Copy link
Contributor Author

Ok I think something is not working with the vasp tests. (all of them are skipped due to a missing environment variable)

@Felixrccs
Copy link
Contributor Author

I found an error in the tests/prep_test_cli_rss.sh. But in the end the VASP path's are not declared and the pytest is always skipped. I would not be surprised if most of the VASP tests fail if we get this to work again.

Im not familiar how these test pipelines are set up. So I need some help there.

@bernstei
Copy link
Contributor

I found an error in the tests/prep_test_cli_rss.sh. But in the end the VASP path's are not declared and the pytest is always skipped. I would not be surprised if most of the VASP tests fail if we get this to work again.

There's no VASP on the github CI, since it requires a license. Someone has to run the tests manually. I have a local script that does that by setting various env vars. Generally I've been the only one really using VASP a lot, so I've been running that test when anyone submits a PR that's Vasp related. I'm happy to do that for this PR, but it'd also be nice to have a test that's not Vasp-based, so it'll run on the github CI as well.

@Felixrccs
Copy link
Contributor Author

Ok I'll try to do a non VASP pytest.

@bernstei
Copy link
Contributor

Feel free to keep the Vasp test in there as well, and I'll run it locally (in addition to the CI running the non-Vasp one).

@bernstei
Copy link
Contributor

bernstei commented Sep 6, 2023

Thanks. Test looks good, so if it passes, I'll start the merging process (this one into the per-config calculator, and then the per-config calculator into main).

@Felixrccs
Copy link
Contributor Author

Ok I had to change a line, now its done.

@@ -19,7 +19,7 @@ fi

export VASP_COMMAND=vasp.serial
export VASP_COMMAND_GAMMA=vasp.gamma_serial
export VASP_PP_PATH=${VASP_PATH}/pot/rev_54/PBE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this variable? It's internal to ASE's vasp calculator, and this isn't even a test - it's a script that does the reference gap_rss_iter_fit calculation which the corresponding test compares to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a more general discussion of how the VASP tests are configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I first thought that was part of the reason why the pytest for VASP was skipped.
So this change make no sense after since I now that the pytest is not done in the pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - can you undo it please?

@bernstei bernstei merged commit 1e195f4 into libAtoms:autopara_calc_params Sep 6, 2023
1 check passed
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