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

Remove setuptools as runtime dependency #8366

Closed
wants to merge 3 commits into from

Conversation

flying-sheep
Copy link

@flying-sheep flying-sheep commented Aug 18, 2022

It was added in #4794, as you imported pkg_resources at the time.

Since #5209 you no longer import it and therefore no longer have a runtime dependency on setuptools.

@gmarkall
Copy link
Member

Thanks for the PR! We'll look at assigning a reviewer during the next triage meeting (triage meetings occur weekly on Tuesdays)

@flying-sheep
Copy link
Author

Estimated review time: 4 minutes.

@esc esc added the Effort - short Short size effort needed label Aug 18, 2022
@esc esc added this to the Numba 0.57 RC milestone Aug 18, 2022
@esc esc requested review from esc and removed request for stuartarchibald and sklam August 18, 2022 15:00
@esc
Copy link
Member

esc commented Aug 18, 2022

@flying-sheep @gmarkall I will pre-empt this and assign myself as reviewer. Judging from #8356 the reference to setup tools will have to be removed in more places than just setup.py. If we really don't need setuptools anymore that would be wonderful!

@esc esc added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Aug 18, 2022
@flying-sheep
Copy link
Author

Ah, in that case I’m sorry I was so brash with my 5 minute estimate.

You definitely don’t need setuptools at runtime anymore since you don’t import pkg_resources anymore.

But of course I’m not familiar with the code base to know which of all these files are dependencies for runtime or build time. I made a guess, but I’m not sure about e.g. the docs/environment.yml

@esc
Copy link
Member

esc commented Aug 18, 2022

Ah, in that case I’m sorry I was so brash with my 5 minute estimate.

You definitely don’t need setuptools at runtime anymore since you don’t import pkg_resources anymore.

But of course I’m not familiar with the code base to know which of all these files are dependencies for runtime or build time. I made a guess, but I’m not sure about e.g. the docs/environment.yml

Yeah, this may be a bit of a "chisel" to get right. We have multiple ways of building. Though we have enough CI to be reasonably confident.

@esc esc added 4 - Waiting on reviewer Waiting for reviewer to respond to author Effort - medium Medium size effort needed and removed 4 - Waiting on author Waiting for author to respond to review Effort - short Short size effort needed labels Aug 18, 2022
@flying-sheep
Copy link
Author

Are the test timeouts flukes or likely to have something to do with this PR?

I’d assume that tests aren’t supposed to time out if a dependency is missing …

@esc
Copy link
Member

esc commented Aug 18, 2022

Are the test timeouts flukes or likely to have something to do with this PR?

I’d assume that tests aren’t supposed to time out if a dependency is missing …

the latest test of main was all green, so if your branch descends from it's current HEAD then it is likely the failures are caused by this PR.

@flying-sheep
Copy link
Author

I can’t interpret what’s going wrong. I see

INFO: Running slice of discovered tests: (17,None,20)
/usr/bin/strace: Could not attach to process. If your uid matches the uid of the target process, check the setting of /proc/sys/kernel/yama/ptrace_scope, or try again as the root user. For more details, see /etc/sysctl.d/10-ptrace.conf: Operation not permitted
/usr/bin/strace: attach: ptrace(PTRACE_SEIZE, 8843): Operation not permitted

I can of course experimentally re-add the setuptools<60 to the test environment but I don’t understand why or how that kind of error could be caused by the setuptools version.

@flying-sheep
Copy link
Author

OK, that did the trick I guess?

@esc
Copy link
Member

esc commented Aug 20, 2022

OK, that did the trick I guess?

Yeah, looks like the tests pass now. But, does this mean we are not quite ready to drop the setuptools dependency?

@flying-sheep
Copy link
Author

flying-sheep commented Aug 20, 2022

Ah, you still import it in tests. So you are ready to drop it from runtime dependencies, but not from build and test dependencies.

Nushell code:

curl -sL https://pypi.io/packages/cp310/n/numba/numba-0.56.0-cp310-cp310-win32.whl -o /tmp/n.whl
bsdtar -tf /tmp/n.whl
	| lines
	| where $it =~ '\.py$'
	| each { |fn| { name: $fn, content: (bsdtar -xOf /tmp/n.whl $fn | lines | where $it =~ 'setuptools') } }
	| where ($it.content | length) > 0
	| flatten
	| to md

Output:

name content
numba/pycc/platform.py # Need to import it here since setuptools may monkeypatch it
numba/tests/test_import.py # bug in setuptools/packaging causing a deprecation warning
numba/tests/test_pycc.py import setuptools
numba/tests/test_pycc.py setuptools = None
numba/tests/test_pycc.py @unittest.skipIf(setuptools is None, "test needs setuptools")
numba/tests/test_pycc.py def test_setup_py_setuptools(self):
numba/tests/test_pycc.py self.check_setup_py("setup_setuptools.py")
numba/tests/test_pycc.py @unittest.skipIf(setuptools is None, "test needs setuptools")
numba/tests/test_pycc.py def test_setup_py_setuptools_nested(self):
numba/tests/test_pycc.py self.check_setup_nested_py("setup_setuptools_nested.py")
numba/tests/pycc_distutils_usecase/setup_setuptools.py from setuptools import setup
numba/tests/pycc_distutils_usecase/setup_setuptools_nested.py from setuptools import setup

@esc
Copy link
Member

esc commented Aug 23, 2022

@flying-sheep thank you for submitting this and shedding some light on this matter. I am not sure how useful it will be to remove setuptools from Numba at this time. I am not sure what sense it makes to remove it from the runtime dependencies but keep it for test and build time? I think it will make sense to remove setuptools at a later stage, i.e. when we can fully remove it from the code-base. I also know that setuptools has issues and a recent release broke quite a few other prrojects: pypa/setuptools#3505 -- so I guess the question is: what else other that pkg_resources from setuptools do we need and why can we not remove it from the testing requirements?

@esc
Copy link
Member

esc commented Aug 23, 2022

@flying-sheep I just had an OOB conversation with someone about this. It seems like the module distutils is deprecated and that is what we are using now. So we will need to move fully to setuptools and /or packaging at some point anyway. As a result, I would recommend abandoning this PR entirely.

@esc
Copy link
Member

esc commented Aug 23, 2022

Here is the relevant PEP: https://peps.python.org/pep-0632/

@flying-sheep
Copy link
Author

flying-sheep commented Aug 23, 2022

I am not sure what sense it makes to remove it from the runtime dependencies but keep it for test and build time?

If you don’t use setuptools at runtime, it’s just a build tool. A rust application doesn’t need rustc at runtime, a C++ application doesn’t need gcc at runtime. I want to be able to pip install numba without also installing setuptools (pip would download a wheel file)

It seems like the module distutils is deprecated and that is what we are using now.

I’m fully aware of this, but it doesn’t seem relevant to this PR at all.

The motivation for this PR is that pkg_resources is a deprecated part of setuptools and very slow to import. So no matter if you use setuptools at runtime or not, ceasing to import pkg_resources is desirable.

/edit: oops, wrong PR

what else other that pkg_resources from setuptools do we need and why can we not remove it from the testing requirements?

I think that’s the main question here: do you consider numba.pycc an optional feature that is not necessary for numba’s main use case? In this case, users shouldn’t be forced to install setuptools at runtime, and it should be listed as an optional feature extras_require in setup.py or project.optional-dependencies in pyproject.toml, e.g.:

setup(
    ...
    extras_require=dict(
        pycc=['setuptools<60'],
    ),
)

If I understand correctly, users who use numba.pycc will use it in a setup.py anyway, so they will import setuptools there anyway.

@stuartarchibald
Copy link
Contributor

I guess there's two separate concerns here:

  1. distutils is deprecated and replacement is required, setuptools could provide that.
  2. That setuptools is potentially not needed at runtime in the current code base.

I think @esc is alluding to the potential necessity of a distutils replacement in the very near future cf. item 1. The module numba.pycc would need to use this replacement at runtime, which would make it a dependency.

To answer the questions above:

  • From memory pycc doesn't need setuptools just distutils, it's also a non-optional feature. It must work out of the box and provides ahead-of-time compilation capabilities to create Python C-extensions from compiled Python functions.
  • IIRC pkg_resources isn't used anywhere in the current code base so I'm not sure what the concern is?

If I understand correctly, users who use numba.pycc will use it in a setup.py anyway, so they will import setuptools there anyway.

IIRC there's no requirement for this to be the use case, the functionality noted is provided to make integration with setup.py easier.

I want to be able to pip install numba without also installing setuptools

I agree with the sentiment, but am curious as to a use case where this is high impact (with view of needing a distutils replacement very soon!)?

@gmarkall
Copy link
Member

IIUC, we include the test suite with installs of Numba, with the expectation that you can run the tests on any installed Numba version - so if the tests require setuptools, then we would need it to be installed with Numba.

@stuartarchibald
Copy link
Contributor

IIUC, we include the test suite with installs of Numba, with the expectation that you can run the tests on any installed Numba version - so if the tests require setuptools, then we would need it to be installed with Numba.

Thanks for raising this @gmarkall, it's a good point, the test suite does indeed ship with Numba and is expected to be able to run from any installation. The test suite does need setuptools for a small number of tests, but it could be made optional-test-time the same as e.g. needing scipy for running tests in numba.tests.test_linalg.

@flying-sheep
Copy link
Author

IIRC pkg_resources isn't used anywhere in the current code base so I'm not sure what the concern is?

Oh, whoops, I have done too many PRs where I both removed the setuptools dependency and pkg_resources with packaging. This PR indeed doesn’t do that, apologies!

I agree with the sentiment, but am curious as to a use case where this is high impact (with view of needing a distutils replacement very soon!)?

It’s not, I just don’t want to have setuptools installed if it’s not needed, and because of its patching of distutils which I don’t want to happen.

The test suite does need setuptools for a small number of tests, but it could be made optional-test-time the same as e.g. needing scipy for running tests in numba.tests.test_linalg.

To make this explicit you should probably spell it out anyway, something like:

setup(
    ...
    extras_require=dict(
        pycc=['setuptools<60'],
        tests=['scipy', 'numba[pycc]'],
    ),
)

The numba[pycc] here is legal, packages may self-depend. In this case, that means “numba’s extra feature tests depends on pytest, scipy, and the dependencies of its extra feature pycc, i.e. setuptools<60”

@esc
Copy link
Member

esc commented Aug 23, 2022

I have given this issue some more thought and have the following considerations:

We will need to replace distutils with setuptools and/or packaging as per 0.57 anyway since it's been deprecated since Python 3.10, right? I would suggest making this change first since this will allow us to asses if setuptools will indeed become an optional build and test-time dependency. I am not sure how well we can asses what this change will involve. Once we get a clearer understanding and setuptools indeed does become optional, we can encode this dependency explicitly across all supported build systems such as setup.py and conda-build and update the docs to reflect this status. Does this make sense to everyone?

@minrk
Copy link

minrk commented Sep 26, 2022

am curious as to a use case where this is high impact

Chiming in that the presence rarely has an impact (for me), but pinning down to an out-of-date version does have an impact. In particular, it is confusing my students who are writing packages with pyproject.toml, which needs setuptools>=60, but numba is conflicting with that installation.

IMO, upper bounds of just about anything in install_requires should be rare and short-term fixes (or confined to co-developed packages which can expect a coordinated release).

@llimeht
Copy link

llimeht commented Sep 27, 2022

am curious as to a use case where this is high impact

I'm currently looking at a bug that can be fixed with setuptools > 60 but the runtime dependency on an older setuptools prevents me from doing that. (Additional impact, I've spent a couple of hours chasing down why the wrong version of setuptools was ending up in this environment)

@stuartarchibald
Copy link
Contributor

@minrk @llimeht thanks for the input. I'll raise this at the Numba triage meeting today (and at the public meeting if there's no resolution found by the maintainers during triage). The reason for the pinning lies in the explanation given in #8355 (comment), though I think since the time of writing the breaking change in setuptools 0.65 has been reverted, however NumPy's response to use of distutils in numpy>=1.23 will still be that it's recommended to use setuptools < 60.0 for python < 3.12.

@llimeht
Copy link

llimeht commented Sep 27, 2022

Thanks @stuartarchibald; removing the run-time dependency on setuptools for the build-tool still seems like something to investigate regardless.

@stuartarchibald
Copy link
Contributor

From a relatively long discussion about this at both the maintainers' triage meeting and the Numba public meeting, the conclusions are as follows:

Numba is going to attempt the following:

  1. Remove setuptools as a runtime dependency.
  2. Make setuptools an optional runtime dependency for numba.pycc use cases and use the distutils support available from setuptools as needed from there.
  3. There will be no specific version pinning for setuptools under the optional runtime constraint.

@esc
Copy link
Member

esc commented Sep 29, 2022

FWIW: #8474 may be an interesting patch to include here

@stuartarchibald
Copy link
Contributor

Point 2. in #8366 (comment) is implemented (WIP) in #8476, I had to guess quite a bit at equivalent functionality, early reviews/comments are welcomed.

@stuartarchibald
Copy link
Contributor

Point 3. in #8366 (comment) was implemented in #8475 and will be in a 0.56.3 patch release.

@sklam
Copy link
Member

sklam commented Mar 28, 2023

Closing. suggested changes were implemented in PRs noted above.

@sklam sklam closed this Mar 28, 2023
@flying-sheep flying-sheep deleted the patch-1 branch March 28, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on reviewer Waiting for reviewer to respond to author Effort - medium Medium size effort needed
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants