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

[BUG] Conda recipe improvements #1210

Closed
12 of 15 tasks
jakirkham opened this issue Mar 15, 2019 · 13 comments
Closed
12 of 15 tasks

[BUG] Conda recipe improvements #1210

jakirkham opened this issue Mar 15, 2019 · 13 comments
Labels
bug Something isn't working CMake CMake build issue

Comments

@jakirkham
Copy link
Member

jakirkham commented Mar 15, 2019

Am documenting some observations and suggestions after working a little bit with the recipes in RAPIDS. There are some things that could be improved and simplified by leveraging features from conda-build 3. Also there are some build practices from defaults and conda-forge that would be good to adopt to avoid some common issues. My hope is this makes things easier to maintain in the long run. May update this list as things come up. Thoughts on this list would be welcome.

Note: I think these can be saved until things cool down a bit.

@jakirkham jakirkham added Needs Triage Need team to review and classify bug Something isn't working labels Mar 15, 2019
@kkraus14 kkraus14 added code quality CMake CMake build issue and removed Needs Triage Need team to review and classify labels Mar 15, 2019
@kkraus14
Copy link
Collaborator

@jakirkham can a subpackage depend on another subpackage? I.E. can the cudf subpackage depend on the libcudf subpackage? From what I'm seeing it doesn't look possible yet.

@jakirkham
Copy link
Member Author

Yes, this should be possible. Here's an example.

Were there specific issues that you encountered? Is there a recipe I can look at?

@jakirkham
Copy link
Member Author

Have updated the nvcc compiler wrapper item (as there is a proposed solution) and added dynamically linking to cudatoolkit. Though am happy to discuss these further.

@jakirkham
Copy link
Member Author

I'd be curious what the status is here. Also would be willing to help. In particular I think it would be interesting to try and get Conda compilers (and the NVCC wrapper compiler) working on one RAPIDS package (maybe cuDF?) and then see how things go.

@harrism
Copy link
Member

harrism commented Mar 12, 2020

Moving this old bug to 0.14 and assigning @jakirkham to decide if or how to proceed.

@jakirkham
Copy link
Member Author

Sounds good. Thanks Mark! 😄

@vyasr
Copy link
Contributor

vyasr commented Jul 12, 2022

@jakirkham any idea where we stand on this issue now? At least a few of the above issues no longer seem entirely relevant (e.g. using pip) while others have been addressed (the questions around compilers).

@jakirkham
Copy link
Member Author

Certainly things have gotten better, but it looks like there is still more to do here. It will probably require someone to go through this in detail, which I have not yet done, but here are some cursory observations.

From a quick look, it doesn't look like adding pip to requirements/host is still needed. Setting add_pip_as_python_dependency to False would happen in either build scripts or Docker images as we decide (though am not seeing that from a quick search). It appears python x.x is still used in raft (cc @cjnolet), which we should drop. This example is using setuptools in run, but am unclear on whether it is needed. The disabling pip/setuptools downloads was mostly handled upstream (though there may be edge cases like split packages where we need to be mindful conda/conda-build#3993 , which is easy to do). We are still doing python setup.py install, which should be moved to pip install. The package hash is still missing from the build string. We may still want to consider using a split package to build libcudf & cudf and split the pieces out.

Investigating the usage of variants in a build matrix is still an interesting piece of work, but may involve other changes around handling global pinnings. This may require scoping.

So things have definitely improved (particularly on the compiler front), but there is still some room for improvement. Much of this is lower hanging fruit (especially given other changes that have occurred).

@vyasr
Copy link
Contributor

vyasr commented Jul 12, 2022

Is the split not what was done in #10326?

I'm happy to help work through some of these, given all the ongoing build system work I think we'd benefit a lot from cleaning up some of these issues as we go.

@jakirkham
Copy link
Member Author

That's a different kind of splitting. The splitting here is suggesting folding Python & C++ into the same recipe

Yep that's the approach we have taken so far and that seems reasonable

On the variant piece it may be productive to have a meeting with OPS. There has been some discussion about revisiting how pinnings are handled and this fits in with that work

@kkraus14
Copy link
Collaborator

FWIW, I raised an issue (#11119) about my struggles with using multi output recipes and folding cudf and libcudf into the same recipe will just further exacerbate that.

If the conda recipes are treated purely as an implementation detail and user consumption of them is explicitly out of scope, folding them all into a single recipe is somewhat reasonable, though it does limit the ability to parallelize the build process in building conda packages.

@jakirkham
Copy link
Member Author

Thanks for surfacing that. Hadn't seen that issue. Agree that is something we want to take into account. Preferably before doing more splitting (since we will want to extend that to any added outputs)

@vyasr
Copy link
Contributor

vyasr commented May 10, 2024

I just went through the above checklist and the bulk of the items that were previously unchecked were either completed or obsoleted by more recent changes (including the broad shift towards scikit-build/scikit-build-core, the requirement to invoke pip rather than setup.py, the various conda recipe improvements during the CUDA 12 work, etc). The remaining unchecked items are no longer clearly improvements for a variety of reasons. I think we can safely close this issue for now and consider any future improvements in light of the current state of the recipes. Also, we will want to coordinate such work across RAPIDS so that it's not specific to just cudf.

@vyasr vyasr closed this as completed May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake CMake build issue
Projects
None yet
Development

No branches or pull requests

4 participants