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

Update Phonopy and Phono3py installations #324

Merged
merged 17 commits into from
Jul 29, 2024
Merged

Update Phonopy and Phono3py installations #324

merged 17 commits into from
Jul 29, 2024

Conversation

gelzinyte
Copy link
Contributor

No description provided.

@gelzinyte gelzinyte changed the title Updata Phonopy and Phono3py installations Update Phonopy and Phono3py installations Jul 22, 2024
@bernstei
Copy link
Contributor

Thanks. Looks good, let's see what happens. We might want to add the -vvv that the phonopy instructions suggest if the installation does fail, so we can figure out why.

@bernstei
Copy link
Contributor

Looks like the phonopy/phono3py issues are fixed, but it's failing on the numpy version. Need the < 2 here as well, I guess.

@gelzinyte
Copy link
Contributor Author

Do you have any idea why it's still complaining about numpy v2?

@bernstei
Copy link
Contributor

Do you have any idea why it's still complaining about numpy v2?

It's explicitly installed by the CI before wfl is ever installed. That install gets numpy 2.0.1. I suspect that it then installs a version of scipy that is compatible only with numpy 2, but whatever causes numpy to be downgraded to 1.25.3 (as the print statement in the CI log shows) doesn't fix things that are dependent on numpy being 2 rather than 1.

I'm going to push a patch to the CI, and also changed the required version back to just <2. Let's see if it works.

@bernstei
Copy link
Contributor

phono3py appears to be installing numpy 2 (in the CI). I'm investigating.

@bernstei
Copy link
Contributor

OMG, after a lot of fighting I got everything (in particular the phonopy build process) to use numpy<2. I hope the tests pass now.

@bernstei
Copy link
Contributor

bernstei commented Jul 22, 2024

Looks to me like the phonopy issues are fixed, but an example test (mlip daisy chained fitting) is failing. Not sure if we should merge this one and deal with that failure in a separate PR.

[edited] looks like the test error spontaneously improved relative to when we made the reference results that the example compares to

@gelzinyte
Copy link
Contributor Author

I've updated the reference errors, to have all the tests fixed in one go. Any idea why they would have changed at all?

@bernstei
Copy link
Contributor

Any idea why they would have changed at all?

No, not at all. I suppose it might be good practice to chase it down. Unless something in the julia or ACE setup that's changed upstream (it's an ACE fit, right?), I can't think of what might have done it.

@bernstei
Copy link
Contributor

And now something is broken with the pytorch version. Let me investigate further. I hope it's just some change in how it's installed, otherwise I guess we can fix it to an older version.

@gelzinyte
Copy link
Contributor Author

And now something is broken with the pytorch version. Let me investigate further. I hope it's just some change in how it's installed, otherwise I guess we can fix it to an older version.

Thanks!

No, not at all. I suppose it might be good practice to chase it down. Unless something in the julia or ACE setup that's changed upstream (it's an ACE fit, right?), I can't think of what might have done it.

No, it's gap-fit. Comparing the current wfl branch to latest one that passed (md_compressibility_arg) only phonopy (irrelevant to the test) and CI numpy installation has changed, but the actual version is the same (1.26.4).

@bernstei
Copy link
Contributor

No, it's gap-fit. Comparing the current wfl branch to latest one that passed (md_compressibility_arg) only phonopy (irrelevant to the test) and CI numpy installation has changed, but the actual version is the same (1.26.4).

Let me look at the quippy version installed, and see if that might have changed anything. If it's different we can force this one to an earlier version, and see if we restore the older results. If so, we can consult with whoever update quippy as to whether this change is expected.

@gelzinyte
Copy link
Contributor Author

Ha, f90wrap changed from 0.2.14 to 0.2.15

@bernstei
Copy link
Contributor

Ha, f90wrap changed from 0.2.14 to 0.2.15

Yeah, that's also all I found, but I really don't think it should change the results of the fit model. I suppose, assuming that we now got everything else to work, that I can try regressing the f90wrap version in the CI script and see if that restores the previous results. I'll also run the test locally, where I haven't updated quippy in a bit (quippy-ase 0.9.12 and f90wrap 0.2.8) and see if I still get the old results.

@bernstei
Copy link
Contributor

My older installed quip version, with this wfl branch, fails the test, because it gives the older value. I guess that suggests that it really is the quip version. Let me play with those versions and see whether it really is that f90wrap subversion that did it. That would be weird, and disturbing.

@bernstei
Copy link
Contributor

bernstei commented Jul 29, 2024

My local machine is still producing the old daisy chain test values even with the latest quippy and f90wrap. Must be some other update?

@gelzinyte
Copy link
Contributor Author

Simply forcing f90wrap==0.2.14 on GitHub CI gives the newer values.

@bernstei
Copy link
Contributor

Looks like maybe rdkit going from 2023.9.6 to 2024.3.3 changed the values? I certainly see a difference in the precise positions in 1.ch.rdkit.xyz compared to running on my own machine, which has an older rdkit version.

@bernstei
Copy link
Contributor

Yes - reverting rdkit to an older version restores the previous test values. I'm happy with the latest rdkit and the new test reference values. I'll delete the debugging workflow, and I think we're ready to merge if you're OK with it, @gelzinyte

daisy chain test results to go back to previous values.  Remove, now
that change has been explained
@gelzinyte
Copy link
Contributor Author

That sounds good! Thanks a lot for figuring this out.

@bernstei
Copy link
Contributor

Merging this one, and we'll need to merge these changes to the other open PRs so that everything passes again.

@bernstei bernstei merged commit 765cc84 into main Jul 29, 2024
5 checks 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