goodchemistryco / Tangelo

A python package for exploring end-to-end chemistry workflows on quantum computers and simulators.
https://goodchemistryco.github.io/Tangelo/
Other
99 stars 27 forks source link

Fixing Pyscf Incompatibility problem #394

Closed anushkrishnav closed 1 month ago

anushkrishnav commented 1 month ago

Fixes #383

The problem inherently lies with PYSCF which they have corrected a few weeks ago, for the time being, my fixes will help remove the issues with the tests, this should be future-proof since it checks if there is any changed in mf_coeff so even if later psf version rolls out with the fix , the code would still work as there must be a change in mf_coeff which will then not trigger the if condition

had a lot of fun working on this issue, initially though it was something to do with the data structure of mo_coeff then saw it was a pyscf issue

@alexfleury-sb or @ValentinS4t1qbit Hopefully this would help close the issue for now

ValentinS4t1qbit commented 1 month ago

Maybe @alexfleury-sb has suggestions on how to handle the fix, thanks to your investigation. Alexandre, the automated tests currently force PySCF 2.4.0: do we have a clever way to enable @anushkrishnav to run the test suite on 2.5.0 in his PR ?

@anushkrishnav Before giving you a review, I think you should look at the "checks" at the bottom of this page, click "details" and see what tests are failing and why. Right now, the failures suggest that things are not working for whatever version of pyscf is used in the automated tests (see above). You have an easy way to check the tests locally on your machine: check out the README to see how to run tests locally with unittest, and swap versions of PySCF to see if it works for both.

It is btw perfectly acceptable to have a conditional statement that literally tests the version of pyscf at runtime and does 2 different things depending on the version detected (e.g before and after 2.5.0?). I'd rather be clear and explicit about why things are done, otherwise we'll end up with unexpected instructions for which we do not really remember the purpose.

alexfleury-sb commented 1 month ago

I am fairly positive that changing https://github.com/goodchemistryco/Tangelo/blob/6791dc83d9dac7ee5aad9d5a1f3dcbed81aac2cb/.github/workflows/continuous_integration.yml#L76 would indeed change the pyscf in the CI for this specific PR. We could change it to something like pyscf>=2.5.0 to make sure it is not going back to 2.4.0 for whatever reasons I can't think of at the moment (unless there are mechanisms in the code to handle the pyscf versions at runtime).

anushkrishnav commented 1 month ago

The investigation took the most time tbh,

It is btw perfectly acceptable to have a conditional statement that literally tests the version of pyscf at runtime and does 2 different things depending on the version detected (e.g before and after 2.5.0?). I'd rather be clear and explicit about why things are done, otherwise we'll end up with unexpected instructions for which we do not really remember the purpose. (I can add a comment on that to make things clear )

@ValentinS4t1qbit We don't really need a before and after, the whole thing was caused by a bug from pyscf side, In the issue I have detailed what going on, and why this error happened in the first place. , This issue has now been fixed from pyscf side, I guess the next version will have that fix, this issue that we had was not a feature rather a bug from their side

https://github.com/pyscf/pyscf/commit/9a152a9953f58bc632cb873bc6f9971e36216015

anushkrishnav commented 1 month ago

So my next step will be the testcases rt ? I can try and toggle between version locally, I was using 2 machines to trace the error using the 2 version, Is there a better way to do it ? I think rt now the issue is we are not yet able to run tests on 2.5 rt ?

ValentinS4t1qbit commented 1 month ago

@anushkrishnav Thank you for investigating this ! Sorry for the delay, weekend + international travels.

Local testing : I think an easy way to test if your code works for both is simply to test in your local environment one version, then uninstall pyscf with pip uninstall -y pyscf and then install the other version with pip install pyscf==x.y.z. We do not know what versions of pyscf our users rely on and do not know when the new pyscf version with the fix will be released: i'd rather we have a conditional statement for now (if pyscf.__version__ < 2.5.0 or something like that).

Automated tests on Github: In order for automated tests to run with the desired version of pyscf, you can modify the yml workflow file in .github/workflows/continuous_integration.yml. If you scroll down, you'll see that @alexfleury-sb enforced pip install pyscf==2.4.0 after we saw the latest version was troublesome. Here I suggest you replace it by pip install pyscf to get the latest (this way when the fix is released, it will be used).

Your goal is for these tests to pass !

anushkrishnav commented 1 month ago

@anushkrishnav Thank you for investigating this ! Sorry for the delay, weekend + international travels.

Local testing : I think an easy way to test if your code works for both is simply to test in your local environment one version, then uninstall pyscf with pip uninstall -y pyscf and then install the other version with pip install pyscf==x.y.z. We do not know what versions of pyscf our users rely on and do not know when the new pyscf version with the fix will be released: i'd rather we have a conditional statement for now (if pyscf.__version__ < 2.5.0 or something like that).

Automated tests on Github: In order for automated tests to run with the desired version of pyscf, you can modify the yml workflow file in .github/workflows/continuous_integration.yml. If you scroll down, you'll see that @alexfleury-sb enforced pip install pyscf==2.4.0 after we saw the latest version was troublesome. Here I suggest you replace it by pip install pyscf to get the latest (this way when the fix is released, it will be used).

Your goal is for these tests to pass !

I have already tested mine on local, I installed and uninstalled the package to see how it performs. I will now move to the automated tests, is it something that I can do ? updating the workflow?

ValentinS4t1qbit commented 1 month ago

I have already tested mine on local, I installed and uninstalled the package to see how it performs. I will now move to the automated tests, is it something that I can do ? updating the workflow?

Yes, that is what the 2nd paragraph of my previous answer was suggesting

anushkrishnav commented 1 month ago

Updated the CI file

anushkrishnav commented 1 month ago

Not sure how do I navigate the issue, there has not been any change except the one made to get it back to older state , when on newer version, this rectifies the mistake made on pyscf side. I am not sure about the final test, it is supposed to be skipped on pyscf rt ? @ValentinS4t1qbit

anushkrishnav commented 1 month ago

@ValentinS4t1qbit This is a good solution to our previous problems. As I read in the thread, the bug was fixed on pyscf side and is only appearing with version 2.5.0.

For the MINDO tests that fail, I can take care of them! I think this is out of scope for this PR.

Awesome , So hopefully this is a wrap for the problem ?

anushkrishnav commented 1 month ago

Thank you for the help and guidance @ValentinS4t1qbit