scottrini / OctoPrint-PrusaLevelingGuide

42 stars 7 forks source link

relax numpy/scipy version spec - fixes #34 #35

Closed draeath closed 3 years ago

draeath commented 3 years ago

This should resolve #34 - I tested and installation without any version spec for numpy/scipy (so latest versions were selected) and was able to complete a bed leveling procedure without any apparent malfunction or error (in UI or in logs).

That said, some level of conservativeness is appropriate so in this PR I have locked the versions using the ~= version specification so that it will take future patch releases, but not new major or minor versions.

Ie, if scipy 1.7 came out this would NOT be selected, but scipy 1.6.* would.

scottrini commented 3 years ago

Hey there, thanks for taking a look at this because I was going to revisit it myself after seeing all the new issues. These new dependencies are from the first external PR I merged that added support for other firmwares. Originally, this plugin only relied on regex and was a bit easier to maintain.

I pinned to these versions by someone else's suggestion, see here: https://github.com/scottrini/OctoPrint-PrusaLevelingGuide/pull/30#issuecomment-779588987

I think the reason he mentioned these versions is that they still support python2.x. I understand switching to a more lenient pin to allow patch upgrades since they shouldn't introduce breaking changes, but I was more aligned with finding an exact pin of versions we can sure would continue to work for python2.x and python3.x.

So let's start there - are you certain those versions you pinned are compatible with python2.x?

Again, I appreciate another set of eyes on this. I was skeptical to merge the PR with the new dependencies, but got it working on some local instances, so finally merged it.

draeath commented 3 years ago

I can do some research and figure out the last versions that had python2 support. I expect the newest don't.

How hard of a requirement is python2 support for you? As you're likely aware it's been sunset/EOL for over a year at this point. People still running python2 really should be upgrading, if possible.

scottrini commented 3 years ago

@draeath I totally agree, they really should. I just don't like the idea of breaking people who have had the plugin installed for a while.

I'm actually starting to lean towards backing out these changes that add these dependencies. scipy really bothers me as a dependency since it's only using a single function. It would be dirty, but I'd be happier just having a functionally identical local method so we can remove the dependency.

draeath commented 3 years ago

Can you point me at where these modules are used? I also wonder if there's a way to accomplish what they're doing, or close enough, without using such a heavy module.

I've skimmed #30 but I'm not seeing it (in fact I see a call to numpy, maybe, being removed?)

scottrini commented 3 years ago

Sorry, that was actually the 2nd PR... This is where the changes came from: https://github.com/scottrini/OctoPrint-PrusaLevelingGuide/pull/29

This is the only line where scipy is used: https://github.com/scottrini/OctoPrint-PrusaLevelingGuide/pull/29/files#diff-2174b7ba682fe6343d0e927cb43674e54c4749a487bac6f4fb1db6344a896928R90

wolph commented 3 years ago

I imagine Python 2 support is important for many octoprint users since it is (was?) the default. I installed octoprint brand new about 6 months ago and it came with Python 2 by default.

So... the change broke the plugin for me as well. I have since upgraded to Python 3 but it still won't install properly because it now tries to build scipy from source which is quite a challenge for a pi3 ;)

Quick workaround for people btw:

~/oprint/bin/pip install --no-deps 'https://github.com/scottrini/OctoPrint-PrusaLevelingGuide/releases/download/1.0.14/OctoPrint-PrusaLevelingGuide-1.0.14.zip'
draeath commented 3 years ago

It looks like the most recent numpy/scipy versions available for python 2.7.18 are:

numpy==1.16.6
scipy==1.2.3

Unfortunately this is not something we can rectify with systems with gcc suite 10 (or newer) as scipy earlier than 1.5.0 will fail to build.

So we need to make a choice here:

  1. Keep #29 and require gcc suite earlier than 10 (an undue burden on users in my opinion, as this is usually not easy to control)
  2. Keep #29 and drop python 2.7 support
  3. Revert #29
  4. Find a way to accomplish what #29 is doing with numpy/scipy without using said modules.

Regarding the fourth choice, I can't tell exactly what it's doing, though the docs suggest it can be done for either smoothing or interpolating points on a mesh. I don't have any suggestion on how one might do this without that module - such computational work is outside of my experience.

The nearby call to np.linspace() looks like something that could be replaced with a function even I could write (I say now, ask me again a few hours into trying :D) but fortunately numpy does not require scipy's presence and is also not subject to the gcc suite 10 thing. Additionally wheels for aarch64 linux exist for numpy so, at least in my case, you don't actually have to build it.

scottrini commented 3 years ago

@draeath So 3/4 were already what I was leaning towards. Once I got a moment, I was going to revert the changes so existing people using the plugin would be fixed and unblocked from using it.

But I think the only way I'd merge the changes back in is if I found a way to eliminate scipy altogether. numpy seems more widely used and even the issues I had were related to scipy.

draeath commented 3 years ago

@WoLpH the workaround is dangerous - if the code path discussed here is entered, the plugin will throw ImportError: No module named ... exceptions and die unless you get the regex (easy), numpy (easy), and scipy (the troublemaker) modules installed yourself.

For what it's worth, installing octoprint on python3 Just Works (up to 3.8.x at least, i haven't tried 3.9.x), and octoprint's configuration backup/restore system doesn't seem sensitive to the python environment (excepting the Print Time Genius plugin, that I know of, which stores absolute paths to the interpreter and some .py files for some reason). Alternatively the .octoprint directory doesn't need to be removed either, so you can "just" replace your octoprint venv with one built on python 3, install any plugins you use, and away you go.

draeath commented 3 years ago

@scottrini I'll close this PR as either of those solutions make it irrelevant. Thanks for the discussion :)

wolph commented 3 years ago

@WoLpH the workaround is dangerous - if the code path discussed here is entered, the plugin will throw ImportError: No module named ... exceptions and die unless you get the regex (easy), numpy (easy), and scipy (the troublemaker) modules installed yourself.

Fair enough. But unless I let my pi compile scipy it doesn't currently work for me. It didn't for python 2 and it still doesn't after following the official python 3 upgrade guide for octoprint.

scottrini commented 3 years ago

@WoLpH Upgrade to the latest release I cut a few minutes ago, I think it was 1.0.15. It's basically a rollback and doesn't require those dependencies.

sourekv commented 3 years ago

1.0.15 works for me. Thanks.

wolph commented 3 years ago

@WoLpH Upgrade to the latest release I cut a few minutes ago, I think it was 1.0.15. It's basically a rollback and doesn't require those dependencies.

Yes! works great :)