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

Use ResourceResolvers in lammps/vasp/sphinx #1527

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Use ResourceResolvers in lammps/vasp/sphinx #1527

merged 11 commits into from
Aug 16, 2024

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Aug 16, 2024

Minimal changes to make atomistics use the resolvers. See #1528 for clean up.

@pmrv pmrv added enhancement New feature or request format_black reformat the code using the black standard patch backward compatible bug fixes labels Aug 16, 2024
@coveralls
Copy link

coveralls commented Aug 16, 2024

Pull Request Test Coverage Report for Build 10417795334

Details

  • 45 of 53 (84.91%) changed or added relevant lines in 5 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.002%) to 70.93%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/vasp/potential.py 6 7 85.71%
pyiron_atomistics/sphinx/base.py 0 2 0.0%
pyiron_atomistics/atomistics/job/potentials.py 23 28 82.14%
Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/sphinx/base.py 1 80.04%
pyiron_atomistics/vasp/potential.py 1 89.6%
Totals Coverage Status
Change from base Build 10394386171: -0.002%
Covered Lines: 10680
Relevant Lines: 15057

💛 - Coveralls

@pmrv pmrv requested review from jan-janssen and samwaseda August 16, 2024 06:56
@pmrv
Copy link
Contributor Author

pmrv commented Aug 16, 2024

Test failure was due to the backwards version testing. I've updated environment-old.yml, but not sure if that's all it takes to propagate that change to packaging.

@samwaseda samwaseda added the integration Start the notebook integration tests for this PR label Aug 16, 2024
Copy link
Member

@samwaseda samwaseda left a comment

Choose a reason for hiding this comment

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

If it passes the integration tests I'm fine.

Copy link
Member

@jan-janssen jan-janssen 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

pmrv added 7 commits August 16, 2024 11:18
Otherwise we do not get to import the resolvers.
base 0.9.12 needs the resolvers as well, so it makes no sense to have this
bound lower.
This requires a new class attribute resource_plugin_name, so that it
does not have to passed to _get_resolver anymore
@pmrv
Copy link
Contributor Author

pmrv commented Aug 16, 2024

Like I wrote in #1528 and I added those commits here now as well.

@jan-janssen Is changing environment-old.yml enough to update the version bounds in the packages or do I need to change something else as well?

@jan-janssen
Copy link
Member

@jan-janssen Is changing environment-old.yml enough to update the version bounds in the packages or do I need to change something else as well?

Updating the environment-old.yml is sufficient. The information is combined in the https://github.com/pyiron/pyiron_atomistics/blob/main/.ci_support/release.py script which is called in https://github.com/pyiron/pyiron_atomistics/blob/main/.github/workflows/deploy.yml

@pmrv pmrv merged commit 9925f73 into main Aug 16, 2024
23 of 24 checks passed
@pmrv pmrv deleted the resolv_base branch August 16, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request format_black reformat the code using the black standard integration Start the notebook integration tests for this PR patch backward compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants