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

[Dev] Multiple dependencies for CI missing #3684

Closed
10 of 13 tasks
DanielYang59 opened this issue Mar 10, 2024 · 6 comments · Fixed by #3786
Closed
10 of 13 tasks

[Dev] Multiple dependencies for CI missing #3684

DanielYang59 opened this issue Mar 10, 2024 · 6 comments · Fixed by #3786
Labels
dependencies Dependency issues and PRs needs discussion Needs discussion to agree on actionable next steps tests Issues with or changes to the pymatgen test suite

Comments

@DanielYang59
Copy link
Contributor

DanielYang59 commented Mar 10, 2024

Multiple external dependencies are missing from test setup, including:

Added

Not planned

@janosh
Copy link
Member

janosh commented Mar 11, 2024

i believe missing icet was deliberate as @esoteric-ephemera added it only to requirements-optional.txt which is not used in CI.

though i could see how that is confusing which is why i've advocated for removing the requirements files in the past. @shyuep prefers to keep. i fwiw, i think the atomate2 approach is better of having a strict optional deps section in pyproject.toml

@janosh janosh added dependencies Dependency issues and PRs tests Issues with or changes to the pymatgen test suite needs discussion Needs discussion to agree on actionable next steps labels Mar 11, 2024
@janosh
Copy link
Member

janosh commented Mar 11, 2024

emmet is purposefully not installed as it comes with a host of transitive deps.

as for the others, i think it would be good to install and test their respective pymatgen integrations in CI

@esoteric-ephemera
Copy link
Contributor

i believe missing icet was deliberate as @esoteric-ephemera added it only to requirements-optional.txt which is not used in CI.

That was deliberate. The same logic applies to mcsqs and a few other packages here. Tho that tends to mean that tests for these aren't actively maintained since CI never runs them (could suddenly have broken tests / interfaces if breaking changes are made in a dependency)

@DanielYang59
Copy link
Contributor Author

Tho that tends to mean that tests for these aren't actively maintained since CI never runs them (could suddenly have broken tests / interfaces if breaking changes are made in a dependency)

But I assume it's still beneficial to keep the tests alive for robustness? (Issues are more likely to slip in if tests are skipped)

Anyway, I would try to install some of these dependencies locally and see if any broken tests show up, and I would keep you updated.

@janosh
Copy link
Member

janosh commented Mar 13, 2024

@DanielYang59 feel free to install some of the missing packages in CI as well to activate those tests. ideally, we don't want to be blocked by upstream packages in our development (see e.g. tblite) but if a package is actively maintained and easy to install, it makes sense to add it in CI

@DanielYang59 DanielYang59 changed the title [Dev] Multiple dependencies for unit tests are missing [Dev] Multiple dependencies for CI missing Apr 15, 2024
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Nov 30, 2024

A very late update, I ran tests for mcsqs locally and looks like it's not aging too badly and only one timeout exception test is failing

I haven't investigated on the design of mcsqs_caller but it seems to raise three different timeout errors. I would just replace the error message in #4209 as both points to mcsqs timeout, let me know if I'm misunderstanding anything

try:
for process in mcsqs_find_sqs_processes:
process.communicate(timeout=search_time * 60)
if instances and instances > 1:
process = Popen(["mcsqs", "-best"])
process.communicate()
if os.path.isfile("bestsqs.out") and os.path.isfile("bestcorr.out"):
return _parse_sqs_path(".")
raise RuntimeError("mcsqs exited before timeout reached")
except TimeoutExpired:
for process in mcsqs_find_sqs_processes:
process.kill()
process.communicate()
# Find the best sqs structures
if instances and instances > 1:
if not os.path.isfile("bestcorr1.out"):
raise RuntimeError(
"mcsqs did not generate output files, "
"is search_time sufficient or are number of instances too high?"
)
process = Popen(["mcsqs", "-best"])
process.communicate()
if os.path.isfile("bestsqs.out") and os.path.isfile("bestcorr.out"):
return _parse_sqs_path(".")
os.chdir(original_directory)
raise TimeoutError("Cluster expansion took too long.")

raise RuntimeError("mcsqs exited before timeout reached")

raise RuntimeError(
"mcsqs did not generate output files, "
"is search_time sufficient or are number of instances too high?"

raise TimeoutError("Cluster expansion took too long.")

Update: also tested installing it in CI 3f63f4b but it's not worth it (way too slow at ~90 s)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Dependency issues and PRs needs discussion Needs discussion to agree on actionable next steps tests Issues with or changes to the pymatgen test suite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants