aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
7 stars 17 forks source link

clean up dependencies requirements #395

Closed unkcpz closed 1 year ago

unkcpz commented 1 year ago

The principle of my cleaning process is the packages in the list do not conflict with each other and do not duplicate. For example, if the package is installed by aiida-core dep, it should not be specified again in the list. As for the jupyter backend packages, there is no need to install then, since

aiida-core > 2 will lead to install numpy~=1.19 therefore numpy~=1.23 is installed.

The following packages are even in conflict and will override the deps which causes the Jupyter engine to fail, we need to pining version when we find future confliction.

unkcpz commented 1 year ago

The test failed as expected, it rely on https://github.com/aiidalab/aiidalab-docker-stack/pull/333

danielhollas commented 1 year ago

I wonder was there a specific reason for this constraint? nbconvert <6 Do you know @yakutovicha? Are we risking breakage when nbconvert is updated?

yakutovicha commented 1 year ago

I wonder was there a specific reason for this constraint? nbconvert <6 Do you know @yakutovicha? Are we risking breakage when nbconvert is updated?

I remember nothing about this package. One needs to test...

yakutovicha commented 1 year ago

I wonder was there a specific reason for this constraint? nbconvert <6 Do you know @yakutovicha? Are we risking breakage when nbconvert is updated?

I remember nothing about this package. One needs to test...

btw, there is also https://github.com/aiidalab/aiidalab-widgets-base/issues/384.

danielhollas commented 1 year ago

Thanks @yakutovicha, I was not aware of that issue. Couple days ago I also opened #392 which is related.

unkcpz commented 1 year ago

@unkcpz could you open a new PR which would only fix the dependencies for the current Docker stack, listed above? aiida-core[atomic_tools] is a separate discussion.

Sure, updated. I add PyCifRW explicitly since it is required by this package.

unkcpz commented 1 year ago

The firefox tests are expected to fail and PR #397 is for it, there are two chrome tests that also fail but I think if there are two passes then the implementation is correct.. I'll merge this and if anything happened I'll fix it afterward.

yakutovicha commented 1 year ago

Sorry, I am late to the party.

  • aiidalab>=21.11.2 is already pinned in the stack

I am not 100% sure this is a good thing to do. AiiDAlab could be setup outside of the stack, and AWB does require aiidalab package to function.

unkcpz commented 1 year ago

You are right, and I find the doc build always lack the necessary packages if aiidalab package is not explicitly installed. I add aiidalab>=21.11.2 back in https://github.com/aiidalab/aiidalab-widgets-base/pull/397 to fix the doc build test.