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

Problem with Setuptools #1008

Closed
metroid120 opened this issue Feb 1, 2020 · 13 comments · Fixed by #1077
Closed

Problem with Setuptools #1008

metroid120 opened this issue Feb 1, 2020 · 13 comments · Fixed by #1077

Comments

@metroid120
Copy link

metroid120 commented Feb 1, 2020

Hello guys, I have found a problem with setuptools.

In File quantity.py I needed to replace:
from pkg_resources.extern.packaging import version

with
# from packaging import version

I think pkg_resources.extern is no longer provided by setuptools.
This is the first time I am writing something on Github, maybe you can point me to
a resource or tell me what to do in case I find such bugs in open sources projects.

@hgrecco
Copy link
Owner

hgrecco commented Feb 2, 2020

Cannot reproduce. Please provide the versions of the different packages you are using.

@metroid120
Copy link
Author

I am using:
-setuptools 44.0.0
-pint 0.10.2dev branch
-python 3.8.1

@hgrecco
Copy link
Owner

hgrecco commented Feb 6, 2020

I think your installation of setuptools is incomplete as packaging is included within the extern part of setuptools. Are you running this on linux, by chance?

@metroid120
Copy link
Author

Yes I am using linux (arch).

@hgrecco
Copy link
Owner

hgrecco commented Feb 10, 2020

I am pretty sure that you setuptools instalation is broken. packaging is an extern part of setuptools. Maybe this happened or similar. I would suggest you do a clean install. I am closing now, feel free to reopen if needed.

@hgrecco hgrecco closed this as completed Feb 10, 2020
@dev-zero
Copy link

dev-zero commented Apr 7, 2020

Since I hit the very same issue after doing a pip install --user myproject which depends on pint I followed the leads a bit. The cause is that some Linux distributions like Arch Linux have stopped including the vendored packaging package (which was part of setuptools):

So yes, you should directly be using packaging rather than relying on setuptools' import of the packaging library to make it work in any case.

@keewis
Copy link
Contributor

keewis commented Apr 7, 2020

thanks for investigating. My reason for choosing to use pkg_resources.extern.packaging instead of directly using packaging was to avoid adding another (direct) dependency to pint. If that decision causes problems, I guess we should change that (unless there are any objections?).

Do you want to submit a PR?

@jules-ch
Copy link
Collaborator

jules-ch commented Apr 7, 2020

Btw pkg_resources from setuptools can be replaced with standard library importlib.metadata & importlib.resources from Python 3.8 & Python 3.7 respectively.

There are backports for both.

@keewis
Copy link
Contributor

keewis commented Apr 7, 2020

thanks for the tip. Unless I'm missing something, neither of those libraries provide version comparison, though? (distutils does, but neither LooseVersion nor StrictVersion correctly compare the versions setuptools_scm creates)

@dev-zero
Copy link

dev-zero commented Apr 7, 2020

and Duck typing wouldn't work?

    def __matmul__(self, other):
        try:
            # Use NumPy ufunc (existing since 1.16) for matrix multiplication
            return np.matmul(self, other)
        except AttributeError:
            return NotImplemented

@keewis
Copy link
Contributor

keewis commented Apr 7, 2020

numpy.matmul was introduced in 1.10, but it wasn't a ufunc until 1.16 (making it overridable using __array_ufunc__). Since the minimum numpy version pint supports is 1.14, I think we still need the version check (but I might be mistaken).

@jthielen, any thoughts on this?

@jthielen
Copy link
Contributor

jthielen commented Apr 7, 2020

@keewis Indeed, np.matmul's behavior is only correct with Pint Quantities since version 1.16 when it became a ufunc, so checking for an AttributeError in Quantity's __matmul__ would be insufficient. To remove the version check (or to work with NumPy pre-1.16), there would need to be custom unit checking/handling code in __matmul__. When implementing __matmul__ here, I had assumed most people would be on recent NumPy versions, so that it wasn't worth the effort, and the version check was good enough.

However, if the version check needs to be removed, I'm sure a PR would be welcome to do so.

@keewis
Copy link
Contributor

keewis commented Apr 7, 2020

I think it should be enough to add the (currently indirect) dependency packaging as a direct dependency, change the import and remove the version check once we require numpy >= 1.16.

The numpy support for pint>=0.10 heavily depends on recent numpy features, so unless someone has to use matrix multiplication with numpy<0.16 (and maybe puts in a PR), I'd try to avoid increasing the code complexity.

With that said, @dev-zero: do you want to put in a PR? Otherwise I'd do it.

bors bot added a commit that referenced this issue Apr 16, 2020
1077: add packaging to the dependencies r=hgrecco a=keewis

Since a few people reported trouble with the `setuptools` vendoring mechanism (mainly because the ArchLinux package of `setuptools` does not include the vendoring part), this makes `pint` depend directly on `packaging`. We also depend on `setuptools` which in turn depends on `packaging`, but this way the dependency graph is more explicit.

We can revert this once we require `numpy >= 1.16` and are able to remove the version check.

- [x] Closes #1008 (@hgrecco, could you reopen that issue so we can close it?)
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [ ] The change is fully covered by automated unit tests
- [ ] Documented in docs/ as appropriate
- [ ] Added an entry to the CHANGES file


Co-authored-by: Keewis <[email protected]>
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 a pull request may close this issue.

6 participants