-
Notifications
You must be signed in to change notification settings - Fork 18
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
Replace optimade-client with ipyoptimade #553
Replace optimade-client with ipyoptimade #553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I do think we have to workaround the pydantic versions somehow to support earlier aiida-core versions
setup.cfg
Outdated
@@ -20,7 +20,7 @@ classifiers = | |||
packages = find: | |||
install_requires = | |||
PyCifRW~=4.4 | |||
aiida-core>=2.1,<3 | |||
aiida-core>=2.5,<3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be very careful about bumping the minimum aiida-core version since it might cause issues for downstream apps. If at all possible, please make the ipyoptimade compatible with earlier aiida-core versions. Ideally, we should allow pip to pick the correct version based on which aiida-core is already installed in the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think very hard and here is some thought.
ipyoptimade
has nothing to do with aiida-core
but with pydantic
.
ipyoptimade<0.2.0
which right after I made the migration, it only supportpydantic==v1.0
ipyoptimade==0.2.0
is the version that support pydantic v2.
So if we need something that need aiida-core==2.5.0
then need pydantic
v2 is the environment and thus need to use ipyoptimade==0.2.0
.
The problem now comes with that the use of optimade widget comes from aiidalab-widgets-base
and it is another base dependencies in an AiiDAlab environment.
- One workaround is not use optimade widget from
aiidalab-widgets-base
but fromipyoptimade
directly.Then nothing need to change here. - Maybe another workaround worth to try is what you mentioned, that the
ipyoptimade
is an extra dependency.
In summary, I think we can not guarantee that one app not break another app. But we need to keep the bottom line that we need to try our best the new version of aiidalab docker-stack will not break the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be very helpful if you have a clear "break path" example such as: update of app-A or component-A would cause the break of some part of app-B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our general goal with AWB is to be as independent of various other core dependencies as possible, to avoid coupling between versions of different dependencies.
ipyoptimade<0.2.0 which right after I made the migration, it only support pydantic==v1.0
ipyoptimade==0.2.0 is the version that support pydantic v2.
This is great that there are diifferent versions that support different pydantic versions! In the best case scenario, we should allow both versions in AWB and let pip
to pick the correct version. Whether that will work in practice is unclear to me, since pip can ignore the existing aiida-core version constraints. This is another reason why we really should publish AWB as conda package and install it together with aiida-core
in the base image.
cec7582
to
13f6ec7
Compare
3312d6c
to
a9bf694
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #553 +/- ##
=======================================
Coverage 87.07% 87.07%
=======================================
Files 27 27
Lines 4649 4649
=======================================
Hits 4048 4048
Misses 601 601
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@unkcpz could you test what happens when you bump the aiida-core version in GHA to 2.5.0?
Otherwise I think this is good to go. Since aiida-core <2.5 versions did not depend on pydantic at all, I think this should be safe. I also double checked that no other packages besides pydantic depend on pydantic.
a9bf694
to
762d55d
Compare
Ah, that's true, there are some deprecation warnings needs to be addressed, but better to do with another PR I think, just to bump the aiida-core version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, cool. Good to see that the installation with 2.5 succeeded at least.
.github/workflows/ci.yml
Outdated
@@ -43,7 +43,7 @@ jobs: | |||
matrix: | |||
browser: [Chrome, Firefox] | |||
python-version: ['3.10'] | |||
aiida-core-version: [2.1.2, 2.4.3] # test on the latest and the oldest supported version | |||
aiida-core-version: [2.1.2, 2.5.0] # test on the latest and the oldest supported version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have that image yet so this was bound to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emmm. Then it is a bit tricky, that one has a test depends on AWB, so we need release docker image first or this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert back to v2.4.3 and merge this first. I think then we might proceed with releasing the image.
Once this is merged, maybe you could make a alpha pre-release of AWB (2.2.0a0) which we can then use for both testing the image and QeApp (which needs to be updated to specify optimade extra dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Releasing the image of bumping aiida-core to 2.5.0 you mean? But the test will fail since awb is not yet support 2.5.0.
This is a chicken egg problem. I think we should make a release of awb without changing the aiida-core version in notebook CI test. Make the release and then update the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not being clear. I agree we can't release the image just yet, and we therefore need to keep the 2.4.3 version the the notebook tests here. Here are the steps that are needed afaik:
- Merge this PR
- Release a pre-release version of AWB
- Make QeApp work with the pre-released version. Verify that it works in the axisting aiidalab images.
- Release new version of AWB, v2.2.0
- Release new version of QeApp
- Release the new image with aiida v2.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarification. But I think it will not working for the step 6.
Release the new image with aiida v2.5
This is impossible if AWB not yet have pydantic v2 supported.
aiidalab docker stack full stack test:
aiida-core 2.5.0 -> pydantic v2 -> aiidalab-qe (pydantic v2 support) -> AWB (pydantic v2 support)
So if not yet AWB pydantic v2 supported
For AWB test, it depend on the image with pydantic v2 supported, but that image depend on the this as shown above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyway, before step 6, I think it is all doable. Let's get this merged and when proceed with can see the potential problem more clear and able to find the solution.
revert python 3.8 deprecation ipyoptimade pinned to ~0.1
762d55d
to
f4bd947
Compare
Able to install with aiida-core v2.5.0 but turns out there are quite a lot things we need to fix.As discussed (see the thread in this PR), will only replace the optimade-client with
ipyoptimade~=0.1
which is the version right after the migration without the support to the pydantic v2 (which comes in versionv0.2
).