-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Use declarative setuptools #7629
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @graingert. Do you have bandwidth to do the same things for dask/dask
? dask/dask#10032
this fails with:
|
c1d9b86
to
ee06f0e
Compare
Hrm, there's usually some delay between packages showing up on the web and them being available for download (something to do with CDN refreshing). I'll restart the conda build CI job to see if it passes now |
Okay, so it's still failing, but got further in the build process EDIT: |
yeah it looks like pip is ignoring the |
it looks like boa disables pip build isolation, https://github.com/mamba-org/boa/blob/cd087c5582e11125fc0a8bc855056de5b4b41ddf/boa/core/build.py#L399-L406 which is needed for pyproject.toml build-system requires to work |
cc @jakirkham @charlesbluca in case you have any thoughts on making |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 26 files + 1 26 suites +1 12h 8m 39s ⏱️ - 20m 6s For more details on these failures, see this check. Results for commit 1280220. ± Comparison against base commit 0d39c19. ♻️ This comment has been updated with latest results. |
There shouldn't be anything to do here |
Yeah, sorry @graingert fixed that in d3106d0 |
00057b2
to
c2e9fc7
Compare
looks like we're getting hit by python/cpython#99130 the recommendation is to use importlib_metadata on platforms with a buggy importlib.metadata.entry_points implementation what do you think @jrbourbeau ? |
Not James, but that seems sensible to me |
But even better! @graingert FWIW using |
8aae43e
to
e6372d5
Compare
b6394dd
to
a49e49d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Thomas! 🙏
Generally this looks good. Had a couple minor questions below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks Thomas! 🙏
9bfc011
to
a57f074
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Thomas! 🙏
This is looking pretty good. Had one minor comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @graingert! Overall this looks great.
distributed/deploy/tests/test_old_ssh.py::test_nprocs_attribute_is_deprecated
and distributed/deploy/tests/test_old_ssh.py::test_old_ssh_nprocs_renamed_to_n_workers
seems to be failing consistently in CI here but also don't appear in the flaky test reports. I opened #7717 to double check that those failures aren't on main
. Maybe some modification was made when copying over the pytest filterwarnings
?
Yeah, it looks like those tests are passing over in #7717. So it appears they're related to the changes here. Again, my guess is it's related to our |
@jrbourbeau nice catch, I diffed the value of None,
0),
('ignore',
- re.compile('(?s)Exception in thread.*old_ssh.*channel\\.send\\(b"\\\\x03"\\).*Socket is closed', re.IGNORECASE|re.DOTALL),
+ re.compile('(?s)Exception in thread.*old_ssh.*channel\\.send\\(b"\\x03"\\).*Socket is closed', re.IGNORECASE|re.DOTALL),
<class 'pytest.PytestUnhandledThreadExceptionWarning'>,
None,
0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @graingert 😄 a couple questions, mostly around conda build stuff:
@@ -27,6 +27,8 @@ requirements: | |||
- python >=3.8 | |||
- pip | |||
- dask-core {{ dask_version }} | |||
- versioneer =0.28 | |||
- tomli # [py<311] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the version selector necessary here? IIRC this is a noarch
build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we build on 3.11 tomli isn't needed, my understanding is noarch builds still build a package for each python version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While true, versioneer has try...except...
logic to prefer the builtin when available. So it probably doesn't matter much whether the selector is here or not
Think Charles' point is we generally don't want to use selectors in a noarch
file since it can't really be conditioned. That said, with build
& host
dependencies this is less relevant. Since those are just used for the build and the packages wind up with a fixed set of dependencies regardless
This all to say there are valid reasons to drop this selector or keep it. However in terms of the end result, it shouldn't matter which way we go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification @jakirkham - I'm fine going either way here, just wanted to verify that this had no impact on the build
-p no:legacypath''' | ||
filterwarnings = [ | ||
"error", | ||
'''ignore:Please use `dok_matrix` from the `scipy\.sparse` namespace, the `scipy\.sparse\.dok` namespace is deprecated.:DeprecationWarning''', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit for readability - is there a reason all of these are triple-single-quoted? Assuming some of these are for character escaping purposes (not too familiar with TOML syntax), but this example pyproject.toml seems to imply that a lot of these could be double quoted instead:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few of these have literal backslash escapes and single quotes in and I wanted the indentation to line up across all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for the clarification, didn't realize that pytest requires literal backslashes in the filter strings. In that case understand that it's probably best to stick to one TOML string type and stay consistent
Any more thoughts on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @graingert for all your work here -- and @jakirkham @charlesbluca for the reviews
Thanks James! 🙏 |
mostly built by running
pipx run setup-py-upgrade .
andpipx run versioneer install --no-vendor
with some manual tweaksrelated to #7622
pre-commit run --all-files