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

Move optimade dependency as extra dependency #554

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Feb 2, 2024

No description provided.

@unkcpz
Copy link
Member Author

unkcpz commented Feb 2, 2024

Here is the attempt of moving optimade as an extra effort #554. I think we want it anyway.

The test failed expect because the aiida-core is update to 2.5.0. I am a bit confused on what we really try to compatible with all these versions pinned?

@unkcpz
Copy link
Member Author

unkcpz commented Feb 2, 2024

A real case in my mind is if the user have a workable AiiDAlab environment with aiida-core==2.4.0, if they upgrade the aiidalab-widgets-base with the pinning of aiida-core>2.1,<3, the aiida-core will not be updated to aiida-core==2.5.

@unkcpz unkcpz force-pushed the de/xx/move-optimade-as-extra-dep branch from 0b4f5a1 to 94b4ba1 Compare February 2, 2024 17:29
@unkcpz
Copy link
Member Author

unkcpz commented Feb 2, 2024

after #556

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (21dfc5d) 87.16% compared to head (4a85ac1) 87.07%.

Files Patch % Lines
aiidalab_widgets_base/databases.py 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
- Coverage   87.16%   87.07%   -0.09%     
==========================================
  Files          27       27              
  Lines        4642     4650       +8     
==========================================
+ Hits         4046     4049       +3     
- Misses        596      601       +5     
Flag Coverage Δ
python-3.10 87.07% <40.00%> (?)
python-3.8 87.10% <40.00%> (?)
python-3.9 87.10% <40.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz unkcpz force-pushed the de/xx/move-optimade-as-extra-dep branch 2 times, most recently from e15be79 to eeb9e15 Compare February 3, 2024 21:24
@unkcpz unkcpz marked this pull request as draft February 3, 2024 23:12
@unkcpz unkcpz force-pushed the de/xx/move-optimade-as-extra-dep branch from eeb9e15 to c5a14fa Compare February 3, 2024 23:24
@danielhollas
Copy link
Contributor

danielhollas commented Feb 3, 2024

Looking at the OptimadeQueryWidget, it seems pretty self-contained and not dependent on other AWB widgets. Therefore, I think it would make sense to push it out of AWB to the new ipyoptimade package.
There might be downsides but imo it's worth exploring.

It would also make it easier for AiiDAlabs Apps to migrate to this package, since it would be more independent of the AWB version (e.g. one could switch to the new ipyoptimade widget even with AWB 2.1.0. (unless there are some optimade dependency clashes).

CC @yakutovicha might have more context here, as he was involved in a decision to move the widget here per #130

@danielhollas
Copy link
Contributor

There's actually a long discussion about whether to include this OPTIMADE widget in AWB on this PR:

#131

I think we need to read through the arguments there and decide if they are still valid (especially given that we want to integrate AWB in the Docker image).

@unkcpz unkcpz marked this pull request as ready for review February 15, 2024 15:37
Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

I think this moves us in the right direction so I think this is good to merge now and iterate on it.

@unkcpz just one request, could you please provide a screenshot of what happens when you try to load QeApp with this version but the optimade_client package is not available?

@unkcpz
Copy link
Member Author

unkcpz commented Feb 15, 2024

image

This is how it looks like. I made a change in the last commit to use <code>optimade-client</code> to format it.

danielhollas
danielhollas previously approved these changes Feb 15, 2024
Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Thanks. I wonder if more formatting would be warranted (e.g. maybe using the bootstrap error class or something like that to get a reddish box around it. Currently it's not immediately clear that it's an error.

Leaving that up to you.

@unkcpz
Copy link
Member Author

unkcpz commented Feb 15, 2024

I agree, it is not so clear. I change it to

image

danielhollas
danielhollas previously approved these changes Feb 16, 2024
Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

aiidalab_widgets_base/databases.py Show resolved Hide resolved
@unkcpz unkcpz force-pushed the de/xx/move-optimade-as-extra-dep branch from be116cf to f5ca2f1 Compare February 16, 2024 00:12
@unkcpz
Copy link
Member Author

unkcpz commented Feb 16, 2024

I quite like the appearance. I feel comfortable when the whole line filled, so I merge it ^^

@unkcpz unkcpz merged commit 089247c into aiidalab:master Feb 16, 2024
9 checks passed
@unkcpz unkcpz deleted the de/xx/move-optimade-as-extra-dep branch February 16, 2024 00:22
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.

None yet

2 participants