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

Use rapids-dependency-file-generator #17

Merged
merged 25 commits into from
May 2, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Mar 25, 2024

Rather than modifying the dependencies, we should delegate the handling of dependencies to DFG. Call out to DFG's newly-stable Python API to modify pyproject.toml.

@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner March 25, 2024 20:59
rapids_build_backend/config.py Show resolved Hide resolved
tests/test_impls.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kyle, this is really great work. It's definitely proof that this integration is possible and is moving us in the right direction. I do think there are some key design issues that we need to address around the rbb/dfg interface before we can get further, particularly around improving dfg's API to support rbb and figuring out what's going to be responsible for producing and appending the alpha version specifiers.

pyproject.toml Outdated Show resolved Hide resolved
rapids_build_backend/impls.py Outdated Show resolved Hide resolved
rapids_build_backend/impls.py Outdated Show resolved Hide resolved
rapids_build_backend/impls.py Show resolved Hide resolved
make_dependency_files(parsed_config, config.dependencies_file, False)
pyproject = utils._get_pyproject()
project_data = pyproject["project"]
project_data["name"] += _get_cuda_suffix(config.require_cuda)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to obey the disable_cuda_suffix config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old version did not obey disable_cuda_suffix - that was only when transforming the dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are going to want the ability to turn on and off the suffix for the package as well. However, this PR is pretty well-scoped right now so let's put a pin in this question, get the PR merged, and then come back to address it. The use-case where I think this will matter is devcontainers (or more generally in integrated dev environments where things are being built from source). We'll just need to ensure consistency.

rapids_build_backend/config.py Outdated Show resolved Hide resolved
rapids_build_backend/impls.py Outdated Show resolved Hide resolved
rapids_build_backend/utils.py Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking awesome Kyle, thanks for going through all the iterations! The changes to dfg have us very close here.

rapids_build_backend/config.py Outdated Show resolved Hide resolved
rapids_build_backend/config.py Show resolved Hide resolved
rapids_build_backend/impls.py Show resolved Hide resolved
rapids_build_backend/config.py Outdated Show resolved Hide resolved
rapids_build_backend/config.py Outdated Show resolved Hide resolved
rapids_build_backend/impls.py Show resolved Hide resolved
rapids_build_backend/impls.py Outdated Show resolved Hide resolved
rapids_build_backend/impls.py Outdated Show resolved Hide resolved
make_dependency_files(parsed_config, config.dependencies_file, False)
pyproject = utils._get_pyproject()
project_data = pyproject["project"]
project_data["name"] += _get_cuda_suffix(config.require_cuda)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are going to want the ability to turn on and off the suffix for the package as well. However, this PR is pretty well-scoped right now so let's put a pin in this question, get the PR merged, and then come back to address it. The use-case where I think this will matter is devcontainers (or more generally in integrated dev environments where things are being built from source). We'll just need to ensure consistency.

tests/test_impls.py Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small issue, but otherwise looks great! I'm pretty happy to start pushing forward with this version.

rapids_build_backend/config.py Outdated Show resolved Hide resolved
rapids_build_backend/config.py Outdated Show resolved Hide resolved
@vyasr vyasr requested review from bdice and jameslamb April 30, 2024 20:13
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall. I have a couple comments. Please take a look at those. I don't think I'll need to review further.

tests/templates/dependencies.yaml Outdated Show resolved Hide resolved
tests/test_impls.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants