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

modernise packaging, and better manage optional depencencies #56

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

liam-o-marsh
Copy link
Contributor

note: this dropped the pinning on both python and the dependencies, please tell me if something doesn't work in a way the tests didn't catch

Copy link
Contributor

@briling briling left a comment

Choose a reason for hiding this comment

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

Thanks a lot Liam!
Questions:

  • I am not sure where the dependencies' versions are taken from -- automatically from requirements.txt?
  • Do you think it makes sense to make [all] the default option and add [minimal] or something that depends only on numpy?
  • I think we should also make pyscf an optional dependency so one can e.g. install [qml] and use B2R2. However this would require more changes of imports I guess. Should it be a separeate PR?

pyproject.toml Outdated Show resolved Hide resolved
qstack/compound.py Outdated Show resolved Hide resolved
qstack/regression/regression.py Outdated Show resolved Hide resolved
@briling
Copy link
Contributor

briling commented Jun 5, 2024

am I doing it wrong?

Collecting git+https://github.com/lcmd-epfl/Q-stack@repackaging-PR
  Cloning https://github.com/lcmd-epfl/Q-stack (to revision repackaging-PR) to /tmp/pip-req-build-o5iewgtn
  Running command git clone --filter=blob:none --quiet https://github.com/lcmd-epfl/Q-stack /tmp/pip-req-build-o5iewgtn
  Running command git checkout -b repackaging-PR --track origin/repackaging-PR
  Switched to a new branch 'repackaging-PR'
  branch 'repackaging-PR' set up to track 'origin/repackaging-PR'.
  Resolved https://github.com/lcmd-epfl/Q-stack to commit c480da86df31ffd94da37d05e8c3cd6e28603c9c
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Requirement already satisfied: numpy in /home/xe/SOFT/miniconda3/envs/qstack-test/lib/python3.9/site-packages (from qstack==0.0.1+v0.0.1.214.gc480da8) (1.22.3)
Requirement already satisfied: scipy in /home/xe/SOFT/miniconda3/envs/qstack-test/lib/python3.9/site-packages (from qstack==0.0.1+v0.0.1.214.gc480da8) (1.10.0)
Requirement already satisfied: pyscf in /home/xe/SOFT/miniconda3/envs/qstack-test/lib/python3.9/site-packages (from qstack==0.0.1+v0.0.1.214.gc480da8) (2.0.1)
Requirement already satisfied: tqdm in /home/xe/SOFT/miniconda3/envs/qstack-test/lib/python3.9/site-packages (from qstack==0.0.1+v0.0.1.214.gc480da8) (4.66.0)
Requirement already satisfied: h5py>=2.7 in /home/xe/SOFT/miniconda3/envs/qstack-test/lib/python3.9/site-packages (from pyscf->qstack==0.0.1+v0.0.1.214.gc480da8) (3.6.0)
Collecting equistore-core@ git+https://github.com/lab-cosmo/equistore.git@e5b9dc365369ba2584ea01e9d6a4d648008aaab8#subdirectory=python/equistore-core (from qstack==0.0.1+v0.0.1.214.gc480da8)
  Using cached equistore_core-0.1.0.dev260-py3-none-linux_x86_64.whl
Collecting sympy (from qstack==0.0.1+v0.0.1.214.gc480da8)
  Downloading sympy-1.12.1-py3-none-any.whl.metadata (12 kB)
INFO: pip is looking at multiple versions of qstack[equio,gmol,qml,regression,wigner] to determine which version is compatible with other requirements. This could take a while.
ERROR: Could not find a version that satisfies the requirement cell2mol; extra == "gmol" (from qstack[equio,gmol,qml,regression,wigner]) (from versions: none)
ERROR: No matching distribution found for cell2mol; extra == "gmol"

@briling
Copy link
Contributor

briling commented Jun 5, 2024

pip install qstack[wigner] git+https://github.com/lcmd-epfl/Q-stack@repackaging-PR works so something wrong with [gmol]?

@liam-o-marsh
Copy link
Contributor Author

* I am not sure where the dependencies' versions are taken from -- automatically from  requirements.txt?

Nothing specific for now... I guess I can specify the versions from requirements.txt as minimal versions?

* Do you think it makes sense to make [all] the default option and add [minimal] or something that depends only on numpy?

not currently possible I think (pypa/setuptools#1503), unless I were to change the build backend from setuptools to something else, but... I'm not sure it's even possible since we have a setup.py file

* I think we should also make pyscf an optional dependency so one can e.g. install [qml] and use B2R2. However this would require more changes of imports I guess. Should it be a separeate PR?

at this point the easier thing to do would be to split qml/ into a separate package called qstack-qml?

@briling
Copy link
Contributor

briling commented Jun 6, 2024

I guess I can specify the versions from requirements.txt as minimal versions?

I think for reproducibility it's better to fix the major and minor versions (e.g. one of our tests fails with pyscf 2.5 because they changed something)

not currently possible I think

Okay! I thought I saw an example how to do it but maybe with an older configuration. Then we'll just need to be clear in the readme

at this point the easier thing to do would be to split qml/ into a separate package called qstack-qml?

Makes sense! Is it possible with minimal refactoring?

@briling
Copy link
Contributor

briling commented Jun 6, 2024

Also there's an upcoming patch concerning the regression module which will lead to some conflicts sorry

@liam-o-marsh
Copy link
Contributor Author

liam-o-marsh commented Jun 6, 2024

Makes sense! Is it possible with minimal refactoring?

Not really. As things are in the python ecosystem, there is no standard way to do this in a single project, so unless we switch the backend to Pantsbuild or whatever, we will have to move qml to a different repo altogether.

Also there's an upcoming patch concerning the regression module which will lead to some conflicts sorry

not a problem, I'm almost used to dealing with this sort of stuff.

I think for reproducibility it's better to fix the major and minor versions (e.g. one of our tests fails with pyscf 2.5 because they changed something)

is it a numerical result specifically?
in any case, yeah I have no issue putting upper bounds on packages, but fixing major.minor seems like overkill.
plus on anything we publish to zenodo we would have a requirements.txt file or something, right?

Okay! I thought I saw an example how to do it but maybe with an older configuration. Then we'll just need to be clear in the readme

yeah I think this is possible in setup.py but honestly it's not an issue if we put that info there. better do that than make users' lives more complicated anyway

@briling
Copy link
Contributor

briling commented Jun 7, 2024

Makes sense! Is it possible with minimal refactoring?

Not really. As things are in the python ecosystem, there is no standard way to do this in a single project, so unless we switch the backend to Pantsbuild or whatever, we will have to move qml to a different repo altogether.

I would like to avoid having a different repo. Can we do it in the way it's done in equistore/metatensor?

I think for reproducibility it's better to fix the major and minor versions (e.g. one of our tests fails with pyscf 2.5 because they changed something)

is it a numerical result specifically? in any case, yeah I have no issue putting upper bounds on packages, but fixing major.minor seems like overkill. plus on anything we publish to zenodo we would have a requirements.txt file or something, right?

Agree with adding upper bounds (can take the current ones I guess except for pyscf but we're planning to change the minimal version to 2.5 soon anyway)

@briling
Copy link
Contributor

briling commented Jun 7, 2024

Well I just think that for "scientific" packages it makes sense to fix versions more strictly (in contrast to e.g. tqdm which just has to be compatible) but I can be wrong

@liam-o-marsh
Copy link
Contributor Author

what did you say pyscf-v2.5.0 broke? I ran the tests both with the minimal versions from requirements.txt and the latest available version from all packages, and the tests give the same results, give or take numerical noise

@briling
Copy link
Contributor

briling commented Jun 7, 2024

Everything passes for me too but earlier either test_otpd.py or test_excited.py used to break! @YAY-C do you remember the problem?

@liam-o-marsh
Copy link
Contributor Author

I would like to avoid having a different repo. Can we do it in the way it's done in equistore/metatensor?

Yes, I think so, let me try.

Okay! I thought I saw an example how to do it but maybe with an older configuration. Then we'll just need to be clear in the readme

yeah I think this is possible in setup.py but honestly it's not an issue if we put that info there. better do that than make users' lives more complicated anyway

also, my bad, it's just not possible to have opt-out dependencies at all.

@briling
Copy link
Contributor

briling commented Jun 7, 2024

Found it: #31

@briling
Copy link
Contributor

briling commented Jun 7, 2024

My check was wrong! Tried again and test_otpd.py failed
environment.yml.txt

@liam-o-marsh
Copy link
Contributor Author

...right, of course CI breaks down.

@YAY-C
Copy link
Contributor

YAY-C commented Jun 7, 2024

Everything passes for me too but earlier either test_otpd.py or test_excited.py used to break! @YAY-C do you remember the problem?

so for linux user's, I think there's no problem!
From my ubuntu machine at work and home I can install qstack from pip install git+https://github.com/lcmd-epfl/Q-stack.git@master and all the tests work fine!

test_otpd.py breaks if you use a newer version of pyscf (explicitly pyscf==2.2.0 ,vs the required pyscf==2.0.1)
but for now this is only relevant for MacOS users, because for some reason (C-compiler issues) you can not compile pyscf==2.0.1

@briling
Copy link
Contributor

briling commented Jun 7, 2024

I will take a look later but looks like a big difference, beyond normal round off errors

@liam-o-marsh
Copy link
Contributor Author

wouldn't be surprised if it was a change in the way the grid points are placed or something

@liam-o-marsh
Copy link
Contributor Author

liam-o-marsh commented Jun 7, 2024

it's not the grid, and it's not libxc (it seems)
the 2.2.0 release notes, however, mention a change in the DIIS algorithm, so in case the DFT convergence tolerence isn't super tight...
edit: nope, tightening tolerances doesn't do anything

EDIT2: yes it actually does: tightening the rks.conv_tol and rks.conv_tol_cpscf tolerances by 1E3 makes so that the results in pyscf==2.2.0 and pyscf==2.1.1 are much much closer to each other than before

@briling
Copy link
Contributor

briling commented Jun 7, 2024

yeah this is what i was thinking about too! thanks a lot

@liam-o-marsh
Copy link
Contributor Author

liam-o-marsh commented Jun 10, 2024

...can I have the ability to force-push in this repackaging-PR branch, please? So I can replace the final commit over and over to fix the CI

@briling
Copy link
Contributor

briling commented Jun 10, 2024

I can't find how to do it. This branch isn't protected...
Maybe you push as much as you want and then we squash&merge?

@liam-o-marsh liam-o-marsh force-pushed the repackaging-PR branch 7 times, most recently from c166141 to c7b1106 Compare June 11, 2024 08:20
@liam-o-marsh
Copy link
Contributor Author

done!

@briling
Copy link
Contributor

briling commented Jun 11, 2024

so cool thanks a lot!

@briling
Copy link
Contributor

briling commented Jun 11, 2024

Can we still have max. version for pyscf? I've remembered that at some point they forced basis set order sorting wrt $\ell$ and it broke my function dealing with orca outputs

@liam-o-marsh
Copy link
Contributor Author

ok!
I want to write a full test for this then. Do you know how to generate the right data to load?

@briling
Copy link
Contributor

briling commented Jun 11, 2024

I changed the function so there's no problem with the new pyscf but who knows what they change in 2.6

@briling briling merged commit 28a16ce into master Jun 13, 2024
2 checks passed
@briling briling deleted the repackaging-PR branch June 13, 2024 15:23
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.

3 participants