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

Fix install instructions #308

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Fix install instructions #308

merged 3 commits into from
Dec 6, 2023

Conversation

j-wags
Copy link
Member

@j-wags j-wags commented Dec 5, 2023

No description provided.

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

Mixing channels really makes me nervous but there's no way around it here - unless one can use the conda-forge::psi4 trick on the command line

Safeguards might involve "psi4=1.8" or #305

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #308 (a553113) into main (4e04c76) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@mattwthompson
Copy link
Member

I might ask to hold off on merging this, though. I've asked the Psi4 folks about the change between psi4::psi4 and conda-forge::psi4 since I find it confusing. We probably will want this change, but it's possible there's a different option that's recommended.

@mattwthompson
Copy link
Member

There's conda-forge::dftd3-python (and conda-forge::dftd4-python, and conda-forge::gcp-correction if those are useful) which is what we're supposed to use instead of anything from the psi4 channel. We might want to do a quick test fit to ensure it actually works. I'm not totally sure why the packaging changed in the channel migration, but this is an iceberg of complexity I'm okay not fully understanding.

@j-wags
Copy link
Member Author

j-wags commented Dec 6, 2023

I'm getting errors when I try to run the example fits the quick start instructions from this PR, so I need to debug more before this is merged.

The dftd3-python and simple-dftd3 packages were the ones raising version errors for Bill Swope (they're only available in version 1.0.0, but psi4 wants 3.2.1). Installing psi4::dftd3, who's latest version is 3.2.1, resolved the errors.

So, still some poking around to do here. Pavan's working env only had dftd3-python and not psi4::dftd3.

@mattwthompson
Copy link
Member

Here's a conversation between Lori and I and earlier today from the #building_packging channel in the Psi4 slack server (I won't ping her here, since this is a tsunami of context without a discrete ask, but if our troubleshooting doesn't work we can formulate another question to bring back to her.)

Me:

How does psi4::dftd3 differ from similarly-named packages on conda-forge? I notice it’s one of the things that used to be pulled down with psi4::psi4 but isn’t there with conda-forge::psi4, so we’ll need -c psi4 which otherwise would not be present. From my searches so far, it seems like it’s used (?) but only if it’s found (i.e. not installed by default) by the conda-forge::dftd3-python package. Not a complaint, just trying to better understand things. (My naivety of the quantum chemistry and the various DFT things out there int he world doesn’t exactly help me here)

LB:

only in older or compatibility-check specs should psi4::dftd3 be pulled down. psi4/psi4#2791 basically, v1.7 allowed the newer c-f ones, v1.8 started preferring them if both available, and v.1.9 stops supporting classic, though it’s not disabling them.

Me:

So is conda-forge::dftd3-python the same thing (or a newer version thereof) as psi4::dftd3? And should I have users pull down conda-forge::dftd3-python?

LB:

well, conda-forge::dftd4-python note d4, not d3, is the same thing (or a newer version thereof) as psi4::dftd4. conda-forge::dftd3-python is a complete rewrite and new API interface to the code that is psi4::dftd3 with a parsed interface. But yes, have users pull down c-f::dftd3-python c-f::dftd4-python c-f::gcp-correction rather than anything from psi4 channel. similar info in:


From this I think conda-forge::dftd4-python is worth a shot. I interpret above to mean that one of these three or so packages needs to be present for something important to Psi4, and it magically works with whatever's available modulo some details of their upgrade schedule. (To be honest I don't understand how or where BespokeFit actually interfaces with Psi4, much less how various .*dftd[3-4].* packages interact with it). My general confusion of why some package did not persist in the conda-forge migration I think is answered by the big table in the linked PR

@j-wags
Copy link
Member Author

j-wags commented Dec 6, 2023

Thanks for the context, that's really helpful.

I wouldn't expect dftd4 to be a replacement for- or superset of- dftd3's functionality, but that's an educated guess with no context so it's very much worth checking.

I think a more evolved version of the table from the PR made it into the psi4 docs. It helpfully puts version numbers to the compatibility ranges.

I can only get a default-qc-spec bespokefit job to work locally (on an M1 mac) with dftd3-python, but Bill could only get it working on his linux cluster with psi4::dftd3. If psi4::dftd3 is present on my mac (even if dftd3-python and simple-dftd3 are ALSO there), the jobs fail for me. For Bill, the jobs failed when only dftd3-python and simple-dftd3 were installed, and only started working once psi4::dftd3 was installed.

gcp-correction is new to me. I don't think it's relevant to us, since I don't see it in Pavan's or my envs, though that could be another avenue to look down.

I need to experiment more with configurations, will try spinning up a linux/intel machine tomorrow to test. Bill said he'd be willing to experiment with other env creation commands if we need him to. Worst case is it's some system library issue on Bill's cluster and we will have to provide a note like "if your system is old you may need to downpin psi4".

@j-wags
Copy link
Member Author

j-wags commented Dec 6, 2023

After some testing, I'm unable to reproduce Swope's issue with the resources I have available. Also, Lori's advice and the psi4 docs indicate that the conda-forge deps should be sufficient. So, I'm going to scope this PR to satisfy the users can get by with psi4 and dftd3 from conda-forge, and make an effort to dig into Swope's case afterwards.

@j-wags j-wags merged commit 2ad28d9 into main Dec 6, 2023
7 checks passed
@j-wags j-wags deleted the install-instructions branch December 6, 2023 23:20
@mattwthompson mattwthompson mentioned this pull request Mar 25, 2024
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