Test Implib.so macOS porting for MOSEK wheel#24270
Conversation
|
@drake-jenkins-bot mac-arm-sequoia-clang-wheel-experimental-release please |
tyler-yankee
left a comment
There was a problem hiding this comment.
cc: @TaylorSasser
+a:@mwoehlke-kitware and +a:@jwnimmer-tri for awareness
@tyler-yankee made 3 comments.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees mwoehlke-kitware,jwnimmer-tri(platform), labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
a discussion (no related file):
working
@jwnimmer-tri can you PTAL at the rules_python / uv changes? In particular, it works fine on macOS, but when I take it for a spin on Linux the wheel builder falls over pretty early in the build. CI will have the logs, or you should be able to see it locally.
MODULE.bazel line 328 at r1 (raw file):
# Python libraries used when building wheels. # "cxxfilt_internal",
working
per @TaylorSasser, cxxfilt was a development/debugging-only dependency, and will be removed soon. Once it's gone, I can revert the commit on this branch that adds it.
mwoehlke-kitware
left a comment
There was a problem hiding this comment.
@mwoehlke-kitware made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
MODULE.bazel line 118 at r1 (raw file):
[ pip.parse( hub_name = "drake_uv",
Why does this replace(?) mypy with uv? I didn't think those were at all the same thing?
At best this seems like maybe it should be its own PR.
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
working
@jwnimmer-tri can you PTAL at the
rules_python/uvchanges? In particular, it works fine on macOS, but when I take it for a spin on Linux the wheel builder falls over pretty early in the build. CI will have the logs, or you should be able to see it locally.
I think the problem is related to genrule(name = "_gen_implib", ...) in the MOSEK build file. When a tool is used in a genrule, it runs in the host configuration instead of the target configuration, and something about the way the rules_python and uv stuff is set up makes that not work correctly.
Take a look at bindings/pydrake/BUILD.bazel and its bindings/pydrake/stubgen.bzl to see the mypy case -- we run it in target configuration (not host). If you wrote a rule() for generating implib instead of a genrule() it would probably work.
There is probably also a way to get the toolchain working correctly for the genrule, but I don't know how to do that off the top of my head.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
MODULE.bazel line 118 at r1 (raw file):
Why does this replace(?) mypy with uv?
Please feel free to correct if I'm misunderstanding (cc: @jwnimmer-tri), but this was my read on this whole setup:
- It happened to be named
drake_mypywhen set up originally (~a few months ago) because it was drake's way of depending onmypy, viauvviarules_python, andmypywas the only dependency (along with its transitives, e.g.librt, getting pulled in) that needed to work this way. - I am now reusing the same
uvandrules_pythonmechanism to also depend onlief(seeuv_requirement("mypy")vs.uv_requirement("lief")). Hence the rename to reflect the details of the manager, not the dependency(ies).
At best this seems like maybe it should be its own PR.
That is fine, this PR only currently exists for testing. In the future once we land this for real, I imagine it would look something like:
[workspace] Update dependency implib_so_internal to <commit w/ Taylor's integrated change>
[wheel] Revise MOSEK lazy loading on macOS using Implib.so
or whatever.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I think the problem is related to
genrule(name = "_gen_implib", ...)in the MOSEK build file. When atoolis used in a genrule, it runs in the host configuration instead of the target configuration, and something about the way the rules_python and uv stuff is set up makes that not work correctly.Take a look at
bindings/pydrake/BUILD.bazeland itsbindings/pydrake/stubgen.bzlto see the mypy case -- we run it in target configuration (not host). If you wrote arule()for generating implib instead of agenrule()it would probably work.There is probably also a way to get the toolchain working correctly for the genrule, but I don't know how to do that off the top of my head.
ah, I wouldn't have realized that, thanks. I will play with this theory tomorrow.
b5d48eb to
5c93277
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
ah, I wouldn't have realized that, thanks. I will play with this theory tomorrow.
Bah, I figured out why my local wheel build wasn't matching CI. I had to docker builder prune to reset the cache. Possibly we could consider fixing this in the wheel builder as a separate effort?
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Bah, I figured out why my local wheel build wasn't matching CI. I had to
docker builder pruneto reset the cache. Possibly we could consider fixing this in the wheel builder as a separate effort?
I've never seen caching logic in the wheel should ever produce a wrong build. I run it all the time after making changes to Drake's source code and it always rebuilds the correct amount. Which Drake file(s) were you changing, where the wheel build didn't account for them?
5c93277 to
610a540
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I've never seen caching logic in the wheel should ever produce a wrong build. I run it all the time after making changes to Drake's source code and it always rebuilds the correct amount. Which Drake file(s) were you changing, where the wheel build didn't account for them?
To clarify on "caching":
Line 53 in 006438f
drake/tools/wheel/image/build-drake.sh
Lines 13 to 20 in 006438f
All of my development on tools/workspace/mosek/gen_implib.bzl has been subject to this; when I was implementing your original suggestion last week I was actually never seeing the output of my new code running. I'm still having to prune on every iteration.
In good news, your original suggestion was spot-on: I've got the Linux wheel working locally and I'll pass it through CI shortly. I've pinged Taylor internally to proceed with upstream when he has a chance. Thanks!
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
To clarify on "caching":
Line 53 in 006438f
drake/tools/wheel/image/build-drake.sh
Lines 13 to 20 in 006438f
All of my development on
tools/workspace/mosek/gen_implib.bzlhas been subject to this; when I was implementing your original suggestion last week I was actually never seeing the output of my new code running. I'm still having to prune on every iteration.In good news, your original suggestion was spot-on: I've got the Linux wheel working locally and I'll pass it through CI shortly. I've pinged Taylor internally to proceed with upstream when he has a chance. Thanks!
Is there a way to key the mount cache based on the SHA of drake-src.tar.gz?
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Is there a way to key the mount cache based on the SHA of
drake-src.tar.gz?
That caching code is correct as written. It's just like in CI or local builds, where the same disk cache persists even as we edit the code. Bazel notices (or should notice) when any actions need to be re-run.
|
@drake-jenkins-bot mac-arm-sequoia-clang-wheel-experimental-release please |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
I've pinged Taylor internally to proceed with upstream when he has a chance.
Remember to do a thorough in-house code review (within Kitware) before sending it upstream. We want to put our best foot forward, right at the start.
@jwnimmer-tri made 2 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
MODULE.bazel line 328 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
working
per @TaylorSasser,
cxxfiltwas a development/debugging-only dependency, and will be removed soon. Once it's gone, I can revert the commit on this branch that adds it.
(Don't forget about this part.)
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee partially reviewed 21 files and all commit messages, and resolved 1 discussion.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
tyler-yankee
left a comment
There was a problem hiding this comment.
(the CI red is just lint)
@tyler-yankee made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
Except for the macOS wheel build, which is real. It would make me feel better about this project if the cxxfilt spurious dep actually got yanked out of here, and if we had fully-green CI including wheels proving that it is feature-complete. |
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
a discussion (no related file):
working macOS CI woes
Still trying+failing to reproduce this locally.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
a discussion (no related file):
working
I need to re-sync the Python venv, because our wheel dependency should be going away; wheel unpack, wheel pack are no longer needed, only used on macOS to do the MOSEK stuff. But, I'd like to rebase on the "routine" updates in #24334 when doing so.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee resolved 2 discussions.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
|
@drake-jenkins-bot mac-arm-sequoia-clang-wheel-experimental-release please |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 6 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
MODULE.bazel line 118 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Why does this replace(?) mypy with uv?
Please feel free to correct if I'm misunderstanding (cc: @jwnimmer-tri), but this was my read on this whole setup:
- It happened to be named
drake_mypywhen set up originally (~a few months ago) because it was drake's way of depending onmypy, viauvviarules_python, andmypywas the only dependency (along with its transitives, e.g.librt, getting pulled in) that needed to work this way.- I am now reusing the same
uvandrules_pythonmechanism to also depend onlief(seeuv_requirement("mypy")vs.uv_requirement("lief")). Hence the rename to reflect the details of the manager, not the dependency(ies).At best this seems like maybe it should be its own PR.
That is fine, this PR only currently exists for testing. In the future once we land this for real, I imagine it would look something like:
[workspace] Update dependency implib_so_internal to <commit w/ Taylor's integrated change> [wheel] Revise MOSEK lazy loading on macOS using Implib.soor whatever.
Yes, the drake_uv (nee drake_mypy) is the "hub name" of a rules_python virtual environment.
The question here is whether we want mypy and implib each in their own hub (i.e., each in their own independent venv) or whether we want to combine the two into a single hub (single venv) as we do here in r6.
If the dependency graphs ever overlapped, we would need them to be a single hub (so that they had a uniform view of e.g. numpy) but since these are both command line tools rather than runtime libraries, they don't necessarily need to be in the same hub. We could leave drake_mypy alone and add a new hub drake_implib.
I don't have a great intuition for whether 1 or 2 hubs will end up being the best answer.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 9 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
MODULE.bazel line 118 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yes, the
drake_uv(needrake_mypy) is the "hub name" of a rules_python virtual environment.The question here is whether we want mypy and implib each in their own hub (i.e., each in their own independent venv) or whether we want to combine the two into a single hub (single venv) as we do here in r6.
If the dependency graphs ever overlapped, we would need them to be a single hub (so that they had a uniform view of e.g. numpy) but since these are both command line tools rather than runtime libraries, they don't necessarily need to be in the same hub. We could leave
drake_mypyalone and add a new hubdrake_implib.I don't have a great intuition for whether 1 or 2 hubs will end up being the best answer.
Thanks for the context! I didn't have a great intuition on that question either, so a couple weeks ago when I wrote it I just picked one for now and went with it. I'm happy to revisit this question after we've upstreamed and come back with the real / non-testing PR.
|
@drake-jenkins-bot mac-arm-sequoia-clang-wheel-experimental-release please |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed all commit messages.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 7 files and all commit messages.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
|
@drake-jenkins-bot mac-arm-sequoia-clang-wheel-experimental-release please |
1 similar comment
|
@drake-jenkins-bot mac-arm-sequoia-clang-wheel-experimental-release please |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
MODULE.bazel line 118 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Thanks for the context! I didn't have a great intuition on that question either, so a couple weeks ago when I wrote it I just picked one for now and went with it. I'm happy to revisit this question after we've upstreamed and come back with the real / non-testing PR.
Right now I'm leaning towards "make it a separate hub, don't rename". The mypy tool and the implib tool are used in very different parts of the build, so if we can keep them more decoupled (by using two different hubs) that seems like a win. Then if e.g. the implib part causes trouble, it will only break wheel CI builds, not from_source builds for users.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri and mwoehlke-kitware).
MODULE.bazel line 118 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Right now I'm leaning towards "make it a separate hub, don't rename". The mypy tool and the implib tool are used in very different parts of the build, so if we can keep them more decoupled (by using two different hubs) that seems like a win. Then if e.g. the implib part causes trouble, it will only break wheel CI builds, not from_source builds for users.
Okay, that works for me; I'll go with that after we upstream to implib and reimplement this PR. (I plan on taking from this branch in chunks, but not merging this branch directly.)
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment and resolved 1 discussion.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
MODULE.bazel line 118 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Okay, that works for me; I'll go with that after we upstream to implib and reimplement this PR. (I plan on taking from this branch in chunks, but not merging this branch directly.)
FWIW It seems to me like the Drake changes will only need to be a single commit (turn on implib on macOS) but we can cross that bridge when we come to it.
still needs venv_upgrade on macOS
|
Just pushing a merge up to latest |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri partially reviewed 18 files and all commit messages.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on tyler-yankee).
Towards #23867
This PR should not be merged, and only exists for visibility + to test wheels in CI. It will be blocked by integrating the changes from
TaylorSasser/Implib.sointo the upstream repository.This change is