Closed larsoner closed 1 month ago
conda-forge-webservices[bot]
commented
2 months ago Hi! This is the friendly automated conda-forge-linting service.
I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.
conda-forge-webservices[bot]
commented
2 months ago Hi! This is the friendly automated conda-forge-linting service.
I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.
Here's what I've got...
For recipe:
.ci_support files and thus will not build any packages.conda-forge-webservices[bot]
commented
2 months ago Hi! This is the friendly automated conda-forge-linting service.
I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.
larsoner
commented
2 months ago Seems to work, for example this pyside6 run produces:
"matplotlib-3.9.1-pyside6_py311h2c9f595_101.conda": {
"recipe": {
"build_variant": "pyside6",
"channel_targets": "conda-forge main",
"python": "3.11.* *_cpython",
"target_platform": "linux-64"
}
},and this pyqt run produces:
"matplotlib-3.9.1-pyqt_py39hb694d8e_201.conda": {
"recipe": {
"build_variant": "pyqt",
"channel_targets": "conda-forge main",
"python": "3.9.* *_cpython",
"target_platform": "linux-64"
}
},
tacaswell
commented
2 months ago Installing both bindings side-by-side is problematic and can lead to conflicts (e.g., on Intel macOS), so conda install matplotlib pyside6 is unsafe.
Installing is broken or importing both is broken? I would have expected it to be possible to install both qt5 and qt6 (but I can see a possible conflict if both want to be called just called "qt"). For pyqt/pyside (against the same Qt version) I expect both to be installable at the same time (and even to both be usable at the same time....so long as you do not pass their objects to each other, but as all the "real" state lives in c++ both bindings work).
larsoner
commented
2 months ago Installing is broken or importing both is broken?
Well really what I've observed is that mne sys_info is broken, but this essentially just imports matplotlib.pyplot and qtpy (via pyvistaqt), both of which should import PySide6 (it's higher priority than PyQt5 for both packages I think?), but this happens on macOS Intel (copied here with some extra newlines to aid readability):
Running MNE's sys_info
Matplotlib created a temporary cache directory at /var/folders/h9/l1shxhdd69nct08ylzq0n6q00000gn/T/matplotlib-_5wuw2by
because the default path (/Users/runner/.matplotlib) is not a writable directory;
it is highly recommended to set the MPLCONFIGDIR environment variable to a writable directory,
in particular to speed up the import of Matplotlib and to better support multiprocessing.
objc[5388]: Class QT_ROOT_LEVEL_POOL__THESE_OBJECTS_WILL_BE_RELEASED_WHEN_QAPP_GOES_OUT_OF_SCOPE
is implemented in both
/Applications/MNE-Python/1.7.1_0/.mne-python/lib/libQt5Core.5.15.8.dylib (0x12a3b12e0) and
/Applications/MNE-Python/1.7.1_0/.mne-python/lib/libQt6Core.6.7.2.dylib (0x130ddd558).
One of the two will be used. Which one is undefined.
objc[5388]: Class KeyValueObserver is implemented in both
/Applications/MNE-Python/1.7.1_0/.mne-python/lib/libQt5Core.5.15.8.dylib (0x12a3b1308) and
/Applications/MNE-Python/1.7.1_0/.mne-python/lib/libQt6Core.6.7.2.dylib (0x130ddd580).
One of the two will be used. Which one is undefined.
objc[5388]: Class RunLoopModeTracker is implemented in both
/Applications/MNE-Python/1.7.1_0/.mne-python/lib/libQt5Core.5.15.8.dylib (0x12a3b1358) and
/Applications/MNE-Python/1.7.1_0/.mne-python/lib/libQt6Core.6.7.2.dylib (0x130ddd5d0).
One of the two will be used. Which one is undefined.
./tools/check_installation.sh: line 86: 5388 Segmentation fault: 11 mne sys_infoThe segfault is maybe expected because of the warnings that precede it.
I haven't worked this out to a minimal example yet. I suspect this means that importing both is problematic, and I need to find out which package is still importing PyQt5, but I'm not sure. However, regardless of the outcome it just seems like it's cleaner and safer to avoid having both frameworks installed together as much as possible to avoid potential problems like this, so there's value in the build selector.
tacaswell
commented
1 month ago If you can get that down to a qtpy + matplotlib import (which I suspect will depend on the import order), if Matplotlib is the second import please report it as a bug to us!
If this is happening on CI, it may be that there is not actually a valid display so mpl is failing when trying to use qt6, then tries qt5. We may need to be more heavy-handed upstream about poisoning trying additional qt variants is something only partially imported.
larsoner
commented
1 month ago If you can get that down to a qtpy + matplotlib import (which I suspect will depend on the import order), if Matplotlib is the second import please report it as a bug to us!
I got on an intel mac and created and env with matplotlib pyqt pyside6 and everything was fine importing matplotlib.pyplot and qtpy in either order. When I manually imported PyQt5.QtWidgets for example then things broke, so it must have been some other package causing the problem by importing PyQt5.
There is a merge conflict so happy to fix this one up if people think build variants are worthwhile, or feel free to close if not
tacaswell
commented
1 month ago Just to check my understanding, conda does not have a notion of "alternatives" where multiple packages can satisfy a need ?
larsoner
commented
1 month ago I don't think so but I'm not an expert in conda so it's possible I've missed it somewhere. But build variants are how I've seen different "flavors" of packages come out, at least for VTK for example when you can choose between vtk=*=osmesa_* or qt variants:
tacaswell
commented
1 month ago It may also be worth re-opening the discussion of changing which version of qt (if any) the matplotlib package depends on. Depending on matpotlib-base + pyside6 should be enough, but I assume that the odds at least one other package will also depend on matplotlib is high.
Given that Qt5 is already EOL (https://www.qt.io/blog/qt-5.15-extended-support-for-subscription-license-holders) we probably should move matplotlib to depend on pyqt6 (or pyside6).
Given that pyside6 is already packaged (there is an open PR to build pyqt6), that the patch to add input hook support will (soon?) land in pyside6, and there are no possibel GPL issues with pyside6 I think there is a strong case.
When I manually imported PyQt5.QtWidgets for example then things broke, so it must have been some other package causing the problem by importing PyQt5.
This suggests this may not be a completely sufficient fix ?
larsoner
commented
1 month ago we probably should move matplotlib to depend on pyqt6 (or pyside6).
PySide6 is the official binding and has a cleaner license (as you point out) so agreed it's a better choice probably. Just for the sake of completeness, PyQt6 might be an option eventually as well, see e.g. https://github.com/conda-forge/pyqt-feedstock/pull/136 (but who knows when that will land!). I'd be happy to close this in favor of migrating to PySide6 as the matplotlib flavor, and document that for people who want to continue with pyqt the workaround is installing matplotlib-base tornado pyqt instead.
I suspect that making this change will cause some problems for people and packaging, but it might be worth doing anyway to move things forward (and to a supported version of Qt!).
I assume that the odds at least one other package will also depend on matplotlib is high.
This might not be as bad as expected -- the conda-forge linter complains if you depend on matplotlib instead of matplotlib-base so hopefully people have listened :)
Given that pyside6 is already packaged (there is an open PR to build pyqt6), that the patch to add input hook support will (soon?) land in pyside6
Upstream it will come out in 6.7.3 but we backported it to conda-forge so it's in 6.7.2 build 1 via https://github.com/conda-forge/pyside2-feedstock/pull/230, so it should be safe to switch at the conda-forge end already.
This suggests this may not be a completely sufficient fix ?
Yeah there are / were probably other packages installed that required PyQt5, such as mayavi (which I recently refactored to have a pyside6 variant for example), pyface, etc.
tacaswell
commented
1 month ago On a bit more thought, I'll support a PR to switch to using pyside6 as the GUI dependency. I suspect that this is something where we need to get signoff from conda-forge core. @larsoner Are you willing to try and run that process rather than this one?
so it should be safe to switch at the conda-forge end already.
Can you pin to a particular build number or better? My memory is that the solvers treated all builds as equivalent (but my memory is 5-8 years out of date).
larsoner
commented
1 month ago Can you pin to a particular build number or better?
I've force-pushed a commit here with >=6.7.2=*_1, not 100% sure if it will work or not. But in theory even pinning to >=6.7.2 should be enough because there shouldn't be any reason conda would pick _0 over _1 (it should prefer the latter).
larsoner
commented
1 month ago @conda-forge-admin, please rerender
tacaswell
commented
1 month ago But in theory even pinning to >=6.7.2 should be enough because there shouldn't be any reason conda would pick _0 over _1 (it should prefer the latter).
To be fair this was a while ago (6years?) but we had the same thought, were running conda create name bunch of packages in ~40 different hosts and then had a few very confused days of things sometimes working and sometimes not which came down to the solve being non-deterministic and picking different builds on different hosts combined with a build-specific bug [if I had to guess the cause I suspect it was from the era of "dict iteration is random" and the solver was taking the first acceptable packaging it saw]. Which is a very long way of saying "I don't trust 'should'" ;) .
larsoner
commented
1 month ago Which is a very long way of saying "I don't trust 'should'" ;) .
Agreed unless a conda person can confirm the pinning in this PR is safe then we could just wait for 6.7.3 to land and then pinning to that should be fine.
Next I guess would be to contact some conda-forge folks to see what their opinion is on matplotlib making this switch?
tacaswell
commented
1 month ago @conda-forge/core This seems like something someone should sign off on before we implicitly change a lot of users to pyside6 !
h-vetinari
commented
1 month ago Just to check my understanding, conda does not have a notion of "alternatives" where multiple packages can satisfy a need ?
Not natively, but it's possible to do with a build-string-based mutex like for openmp.
In any case, for qt-related things, I think we should ping @conda-forge/qt-main, and specifically @hmaarrfk, @jschueller & @ccordoba12.
[...] before we implicitly change a lot of users to pyside6
My first instinct would be that we should at least wait for https://github.com/conda-forge/pyqt-feedstock/pull/136, but I'm not so involved in this space...
hmaarrfk
commented
1 month ago I'm ok with this. I've been trying to ensure that qt-main and qt6-main are co-installable. I think 6.7.2 is aggressive, but users can always downgrade the matplotlib version
ccordoba12
commented
1 month ago Please note that this would prevent Spyder to be installed in the same environment as the latest versions of Matplotlib if Qt 5 and 6 are not really co-installable, which users could find confusing or problematic.
That's because Spyder is not ready for Qt6, much less for Pyside6. The latter is still buggy and unstable (its 6.7.0 version broke star imports, which broke qtpy and all projects that depend on it).
hmaarrfk
commented
1 month ago @ccordoba12 this won't affect anything at runtime for spyder:
On linux:
mamba create --name spyder spyder matplotlib-base "pyside6>=6.7.2" --channel conda-forge --override-channelsWill install:
$ mamba activate spyder
$ spyderseems to open fine
tacaswell
commented
1 month ago The latter is still buggy and unstable (its 6.7.0 version broke star imports, which broke qtpy and all projects that depend on it).
This is a bit worrying.
I think we need a decision on how we are going to make this decision.
From the mpl side, I see no problem and am happy to be "the decider" on it, however given that if there is a blast radius from this it will fall on the CF folks not me I don't think I should be the final say on this PR.
@hmaarrfk can you propose who should make the call (and how!)?
hmaarrfk
commented
1 month ago I don’t think there is a problem with having matplotlib depend on PySide6 and spyder depend on PyQt5
packages should depend on matplotlib-base and that should not bring in any qt dependency.
I and others are very invested in making both python qt binding co-installable.
I say go for it. I’ve been using PySide6 for a while and it seems to be getting better in each version. So if you want it to be the default when you install matplotlib i don’t see a problem with it.
To note, we’ve made open by default to qt6-main
tacaswell
commented
1 month ago To note, we’ve made open by default to qt6-main
What does this mean?
hmaarrfk
commented
1 month ago Re opencv (on phone hard to quote) I’ve built 3 versions, one with qt6 bindings, one with qt5 and one without bindings.
On Linux. If you type
conda install opencv
it should pick out the qt6 bindings since that has a higher build number.
The blast has been “minimal” (I was mostly concerned about end users and not so much maintainers).
Opencv uses qt for their highgui frontend. Not sure how widely used it is.
tacaswell
commented
1 month ago I'll merge this tomorrow (this seems like the thing to do on Friday afternoon!) unless there is a protest.
tacaswell
commented
1 month ago Do I need to do any sort of announcement or documentation?
hmaarrfk
commented
1 month ago Do I need to do any sort of announcement or documentation?
Honestly. Not really. Downstream packages should all depend on matplotlib-base. This is a convenience for your users that read your matplotlib documentation
conda-forge-webservices[bot]
commented
1 month ago Hi! This is the friendly automated conda-forge-linting service.
I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.
Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.
conda-forge-webservices[bot]
commented
1 month ago Hi! This is the friendly automated conda-forge-linting service.
I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.
larsoner
commented
1 month ago @conda-forge-admin, please rerender
github-actions[bot]
commented
1 month ago Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.
This message was generated by GitHub actions workflow run https://github.com/conda-forge/matplotlib-feedstock/actions/runs/10216572449.
larsoner
commented
1 month ago Okay all green and I think this has the selector we converged on:
- pyside6 >=6.7.2 # [python_impl == "cpython" and not osx and not ppc64le]could we instead write "1 OS per line"
- pyside6 >=6.7.2 # [python_impl == "cpython" and win]
- pyside6 >=6.7.2 # [python_impl == "cpython" and linux64]
- pyside6 >=6.7.2 # [python_impl == "cpython" and aarch64]
# We avoid the PySide6 dependency on OSX since they use the native backend by defaultIf you want. You can add a few run constraints to omit the problematic build of 6.7.2 that you all helped resolve upstream
h-vetinari
commented
1 month ago Sorry, I was on a phone before, I hadn't seen @hmaarrfk asking basically the opposite of what I did. I'll let you figure it out -- my preference was as I commented (one line, with an explanatory comment).
As I mentioned in one of the threads further up, I don't think it's worth hacking around the _0 build, just >=6.7.2 is good enough, because the solver has absolutely no reason to ever prefer the old _0 build over _1 (as all their constraints are identical; down to the exact versions from all run-exports; I checked).
larsoner
commented
1 month ago Okay pushed your verison @h-vetinari, okay for you @hmaarrfk ?
h-vetinari
commented
1 month ago We may want to raise a conda issue though. I don't see why the run-constraint to forbid a specific build shouldn't work in principle.
(The one way I can think of that the solver might get tricked into using _0 is if a user already has that pyside build in their environment, but if anyone comes with with such an issue, conda update pyside6 will fix it)
Checklist
Reset the build number to0(if the version changed)conda-smithy(Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)Switch from pyqt (which is stuck on 5.x and no longer getting updates) to PySide6, which is actively updated and has a cleaner license.
I would like to be able to use matplotlib with PySide6 rather than PyQt5. Installing both bindings side-by-side is problematic and can lead to conflicts (e.g., on Intel macOS), soconda install matplotlib pyside6is unsafe. Doingconda install matplotlib-base pyside6 "tornado>=5"is fine, but requires keeping track of what else (if it ever changes) is in thematplotilbrecipe that isn't inmatplotlib-base.So this PR offers a solution -- allow people to do use build selectors likeconda install matplotlib=*=pyside6_*to get full matplotlib but with apyside6binding rather than apyqtbinding.This is really just a proposal-as-PR (that I want to see actually build!), so I'm happy to discuss here first if this even makes sense and close if not the right path forward!(The only real changes here are tometa.yamland the newconda_build_config.yaml, about 10 lines. The rest isconda-smithy rerender -c auto'ed.)