Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
build wheels on CI #619
build wheels on CI #619
Changes from 60 commits
a4e2160
29f5e9d
cdbb0b2
3ec1147
fd29f8e
f2c6323
c2b7e82
6d453ee
1794824
bcb3e19
6036955
94c0e93
e381d11
d932e98
edec7a0
9f371cf
b70fce1
4a1d09e
4c4f4a0
977f672
5f87e2a
6153d1b
cf2ca0e
e872185
9c4d5ce
46254bc
076964a
f22f45c
1d088c7
7ce3653
6a2d6fc
3b058c7
a353e6a
2791228
dff57f8
aba929e
80225dc
6ad0883
8937ab1
9bb33ab
75cd8e8
6a4518d
82a29a6
4d6a369
a76f858
488dfc6
922c5db
44e9847
661c188
5d7bbd9
025a455
c9e8af3
76d858b
ca5059d
9cc4987
33edec4
3daf9ad
5d9645e
4277ae7
588cd24
fe2ebd9
6ebfccf
01dcff6
20df41f
4ca9eff
f65b052
02b8918
c8f0968
c05219f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
suggestion (non-blocking): The alternative would be maintaining a single file and then stripping out the alpha in CMake. Now that it's done this way I don't know if it's worth changing, but it would let you carry around one less file.
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.
Agreed. I did it this way because I am not familiar with the proper CMake syntax to do it. Happy to update it if you have suggestions on how the CMake code would look.
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.
This would probably do it (after the existing
file(STRING...)
command):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, @vyasr, that seems to work! I pushed a commit to remove VERSION_CPP as well as simplify the CI scripts a bit (there was still writing of some unused VERSION file).
There should now be only a single top-level VERSION file (as well as symlink to it from
python/cucim/src/cucim/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.
thought: I was going to ask if this was really necessary since it's CMake's default, but now I see that this is for the
build_local
script. It's out of scope for this PR, but maybe that script should support a default since CMake does?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.
There is a default, but it is "debug" rather than release! Could potentially change it if there is no objection from @gigony
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.
I'll leave that up to you two decide. It does seem odd to me that the default would be a debug build though.
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.
Raised as issue: #629
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.
note: JFYI we've removed this from every other repo at this point but you're welcome to keep it if you find it useful for debugging. It's been a long time since we got much use out of it.
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.
I have now removed it
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.
suggestion (blocking): This shouldn't be necessary. Were you running into specific problems? If so, do we need to address them somewhere?
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.
No, I was originally also installing a newer CMake etc here and was updating pip before doing that. I have now removed this line.
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.
suggestion (blocking): Is this still true? Now that #617 is merged you should be using pyproject.toml. That said, I don't know how the C++ build works when you do
build_local
. Are these dependencies somehow used there? That would be strange and require some gymnastics for library paths, but I suppose it's possible?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.
Yes, I think it still needs to be this way as from the
pyproject.toml
perspective this is a pure Python package. The pyproject build relies on an external build step (build_local
script in this file) having already built and copied the corresponding C++ .so files into thepython/cucim/src/cucim/clara
folder.I agree that this is a bit non-standard and we can potentially look into moving to a scikit-build based approach in the future. I don't have time to focus on that at the moment, though.
One benefit of the approach as it is now is that even Windows users can build the pure Python package using pip without having to have CMake and compile the C++ library (which supports linux only). They would be able to use all of
cucim.skimage
and part ofcucim.core
, but just would not have the I/O functions incucim.clara
available.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.
I get that there's a separate
build_local
run, what I'm confused about is what dependencies are being installed by pip that are used in that step? Is it just for CMake/ninja? If so, are these really "py_build" requirements?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.
Here's what I see on CI:
# To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`. cmake>=3.23.1,!=3.25.0 ninja setuptools>=24.2.0 wheel
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.
Trying deleting these lines in PR: #628
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.
question: I can see why this change was needed in general, but it doesn't seem relevant to this PR. Was it just snuck in out of convenience?
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.
It is needed here or the arm64 wheel builds fail to find libopenslide and fail during the
run build_local
call. The use oflibopenslide
is only needed for C++ lib benchmarks and test cases and it isn't directly used by the Python wheel, but it has to be found to run the C++ build stage (There is not currently a CMake flag to disable building the benchmark binary or tests).This file was deleted.