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

Atomistics causing downstream tests to fail because periodic table can't be found #1184

Closed
liamhuber opened this issue Oct 10, 2023 · 7 comments · Fixed by #1191
Closed

Atomistics causing downstream tests to fail because periodic table can't be found #1184

liamhuber opened this issue Oct 10, 2023 · 7 comments · Fixed by #1191

Comments

@liamhuber
Copy link
Member

liamhuber commented Oct 10, 2023

In pyiron_workflow and pyiron_contrib we now get errors ending like:

File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/factory.py", line 167, in bulk
    return self.ase.bulk(
           ^^^^^^^^^^^^^^
  File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/factories/ase.py", line 49, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/factories/ase.py", line 60, in bulk
    return ase_to_pyiron(ase_bulk(*args, **kwargs))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/atoms.py", line 3111, in ase_to_pyiron
    pyiron_atoms = Atoms(
                   ^^^^^^
  File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/atoms.py", line 122, in __init__
    self._pse = PeriodicTable()
                ^^^^^^^^^^^^^^^
  File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/periodic_table.py", line 218, in __init__
    self.dataframe = self._get_periodic_table_df(file_name)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/share/miniconda3/envs/my-env/lib/python3.11/site-packages/pyiron_atomistics/atomistics/structure/periodic_table.py", line 419, in _get_periodic_table_df
    raise ValueError("Was not able to locate a periodic table. ")
ValueError: Was not able to locate a periodic table.

I have a meeting now, but will poke into it after...

@jan-janssen
Copy link
Member

Presumably we have to install pyiron-data separately, as this is currently not available on pypi and therefore no longer included in the pyiron_atomistics package.

@jan-janssen
Copy link
Member

jan-janssen commented Oct 10, 2023

Testing this hypothesis in pyiron/pyiron_contrib#886 and pyiron/pyiron_workflow#20

@liamhuber
Copy link
Member Author

Testing this hypothesis in pyiron/pyiron_contrib#886 and pyiron/pyiron_workflow#20

Good call, nice pragmatic attack.

The push-pull tests all passed, so I added the run_coverage label on contrib to redo the exact test that failed on the daily workflow.

It sure looks to me like we need the data dependency here in pyiron_atomistics.

@liamhuber
Copy link
Member Author

The push-pull tests all passed, so I added the run_coverage label on contrib to redo the exact test that failed on the daily workflow.

Ah, looks like I actually wanted codeQL. Anyhow, threw that label on too now.

@liamhuber
Copy link
Member Author

So the problem in the downstream repos makes sense, and the solution is easy (add pyiron-data as a dependency here). What I don't understand is how the pyiron_atomistics unit tests passed?

If pyiron-data appeared in the conda environment hierarchy, then the downstream repos would have gotten it installed as well, and it certainly doesn't appear in .ci_support/environment.yml directly. It does appear in .ci_support/environment_notebooks.yml and .binder/environment.yml, but these are not what's being used for the unit tests -- .ci_support/environment.yml is. The only other thing that makes sense to me is that setup.py is pulling it in when the unit tests do pip install, but I don't see anything there that would provide it either.

@jan-janssen
Copy link
Member

In pyiron_atomistics the resources are included in /tests/static https://github.com/pyiron/pyiron_atomistics/tree/main/tests/static

@liamhuber
Copy link
Member Author

In pyiron_atomistics the resources are included in /tests/static https://github.com/pyiron/pyiron_atomistics/tree/main/tests/static

Aha, true. We shouldn't do this. If instantiating our Atoms class requires pyiron-data (i.e. pyiron-resources) or an equivalent, then pyiron-data should be a dependency so that the library works out-of-the-box on downstream packages that depend on it. Also, then we don't need to reproduce the periodic table (and I don't know what else) in tests/static.

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 a pull request may close this issue.

2 participants