-
Notifications
You must be signed in to change notification settings - Fork 8
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
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 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.
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) |
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 still needs to obey the disable_cuda_suffix
config.
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.
The old version did not obey disable_cuda_suffix
- that was only when transforming the dependencies.
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 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.
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 is looking awesome Kyle, thanks for going through all the iterations! The changes to dfg have us very close here.
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) |
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 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.
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.
One small issue, but otherwise looks great! I'm pretty happy to start pushing forward with this 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.
Looks great overall. I have a couple comments. Please take a look at those. I don't think I'll need to review further.
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
.