In terms of licensing this does not help: Just as currently importing pyqt5 means the whole of pyvista needs to conform with gpl, so does importing pyvista_qt, which in turn imports pyqt5. This problem can only be solved by not importing pyqt5 at all in the codebase.

In terms of licensing this does not help: Just as currently importing pyqt5 means the whole of pyvista needs to conform with gpl, so does importing pyvista_qt, which in turn imports pyqt5. This problem can only be solved by not importing pyqt5 at all in the codebase.

It's a reasonable point. Do you propose we move out the entire import? As it stands it's a conditional import that only runs if you have pyvista_qt installed.

I am not the right person to ask this, I am neither an expert on licensing nor affected by the licensing choice of pyvista. From what I read my (!) conclusion is that importing a python module is considered linking, and linking GPL code makes the whole thing subject to GPL terms. So the question here is: Is conditional importing still importing? I'd guess yes. However I'd expect not even experts could be entirely sure, because these cases usually only become clear after litigation. Legal advice will probably stay on the save side usually. The actual judgement would have to be done by downstreams in violation of GPL (e.g. any closed-source downstreams distributing the code), i.e. whether to use pyvista or not and accept any risk, or pyqt5, i.e. whether or not to litigate. Pyvista itself as far as I understand is not in violation of GPL (unless stating the license to be MIT is already considered a violation - it's probably not correct, but by making the pyvista source open it still complies with GPL as far as I understand). So my personal opinion is: Do not import pyqt5 in any way if you want to license pyvista as MIT. However my opinion is not what counts. I seem to remember that closed-source downstreams have been mentioned at some point. If that's correct whoever knows them should ask them, because their opinion matters.

Removing the BackgroundPlotter all together makes sense to me as it completely removes any doubt that pyqt is or isn't being imported. Plus, from a development standpoint I don't have to test an additional dependency.

Codecov Report

Merging #719 into master will decrease coverage by 1.25%.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
- Coverage   84.73%   83.48%   -1.26%     
==========================================
  Files          35       34       -1     
  Lines        9266     8841     -425     
==========================================
- Hits         7852     7381     -471     
- Misses       1414     1460      +46     

I vote for taking all traces of PyQt out of pyvista (this repo) and into a separate repo with a compatible license. We need to focus on the core feature dev and testing to really move the software forward. Also, having things more modular will streamline dev all around and make it easier for new developers to jump on board with PyVsita core.

I've majorly ranted about GPL on many other issues and how we can't even import GPL software without violating the license - so in brief, let's get PyQt out of here and stop worrying about it. Also, having it here is just getting messy.

In practice, we'll need to keep an empty BackgroundPlotter and QtInteractor<whatever_its_called> class around that throw big ol' warnings. I have a feeling this might be a challenge/(pain in the 🍑) for downstream projects like MNE-Python and the now several commercial software packages using the PyQt interface, so we'll definitely want to have all of them (@GuillaumeFavelier / @larsoner and @math-artist are the first to come to mind) fully in the loop before we even think about merging this.

In practice, we'll need to keep an empty BackgroundPlotter and QtInteractor<whatever_its_called> class around that throw big ol' warnings.

Were you thinking of a depreciation warning? We can do that for a minor release, but I'd honestly prefer just forcing people off rather than taking the bandaid off slowly. I've got a closed source project that uses pyvista.BackgroundPlotter and something would generally break between versions anyway, so I had to fix the version until I was ready for a true upgrade. It will be less to maintain as well.

I'd say throwing a depreciation error would be best, but I'll settle with a warning if @GuillaumeFavelier and @larsoner think it's best.

@banesullivan, I'm going to need you to go into add qtdocs.pyvista.org to pyvista_qt-docs. GitHub pages is already enabled, but I need you to setup the domain (or give me access to it).

This is a big change, this plotter is here since I learned about pyvista. I think it's enough for a minor release, what do you think @pyvista/developers ?

I thought the plan was to move towards pyvista-gui though. Is it for even more modularity?

Also, I understand it's called pyvista-qt with a - right? That's what I saw on PyPi. Is there a reason why we should keep the spelling pyvista_qt (with a _) around? Sorry for the naive question, I just think it should be consistent everywhere.

I'd say throwing a depreciation error would be best

I'm in favour of the deprecation error. If it's not there anymore, people should not expect it to work. And we can inform the users about the move. Then this can be removed at the next big release cycle. But if we go for a warning, we should redirect to Plotter as an alternative.

I have a feeling this might be a challenge/(pain in the peach) for downstream projects like MNE-Python and the now several commercial software packages using the PyQt interface

Regarding mne-python, the plan is to pin the latest version of pyvista with the BackgroundPlotter around and work the transition when everything is settled on the new pyvista-qt side. I remain optimistic, hopefully everything will go smoothly 👍

You might be interested @larsoner, @agramfort :)

I'd definitely consider this a minor bump, it's not just a bugfix. Also this seems like a good time to introduce RCs, given there seems to be quite a few affected downstreams that would benefit from early testing.

The underscore vs hyphen thing is a typical problem: Module names must not use hyphens but underscores (if at all), package names should avoid underscores. I.e. the package would usually be pyvista-qt, the module pyvista_qt. Or just make both pyvistaqt to remove any possible confusion: https://www.python.org/dev/peps/pep-0008/#package-and-module-names

I remain optimistic, hopefully everything will go smoothly

Indeed I don't foresee any big issues for MNE, we'll just need to implement some try/except import patterns at our end to accommodate older and newer styles I think

Or just make both pyvistaqt to remove any possible confusion:

I think that sounds like the best solution. It's not the nicest looking name, but it will lead to the least confusion between - and _. I've personally always had issues (especially when downloading from pypi`

Sorry for the naive question, I just think it should be consistent everywhere.

That's my fault. Provided no one has an objection with the name pyvistaqt, I'll change it shortly.

I thought the plan was to move towards pyvista-gui though. Is it for even more modularity?

I think we can slowly add features from pyvista-gui into pyvistaqt. We can start with a solid base and then build up from there.

These changes warrant a MINOR version bump and details should be the top point in the release notes.

Agreed. I've added it to the milestone. We've been due for a minor release anyway.