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.

Sign up for GitHub

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

warn if dependencies file does not exist #33

Merged
merged 4 commits into from
May 15, 2024
Merged

warn if dependencies file does not exist #33

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7cd1f4a
warn if dependencies file does not exist, change default for 'depende…
jameslamb May 10, 2024
0fa5d57
revert default value change
jameslamb May 13, 2024
95f0b19
add comment
jameslamb May 13, 2024
dfde9d3
Merge branch 'main' into warn-missing-file
jameslamb May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions rapids_build_backend/impls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import re
import shutil
import subprocess
import warnings
from contextlib import contextmanager
from functools import lru_cache
from importlib import import_module
Expand Down Expand Up @@ -141,11 +142,19 @@ def _edit_pyproject(config):
if not config.disable_cuda:
cuda_version_major, cuda_version_minor = _get_cuda_version()

# "dependencies.yaml" might not exist in sdists and wouldn't need to... so don't
# raise an exception if that file can't be found when this runs
Comment on lines +145 to +146
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 should distinguish between "dependencies.yaml doesn't exist because it's an sdist" and "dependencies.yaml doesn't exist because the developer forgot to configure it properly." The former is easy to mark with some sort of marker file or a setting that gets written to pyproject.toml.

Copy link
Contributor

Choose a reason for hiding this comment

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

a setting that gets written to pyproject.toml

Maybe we should have this anyway, instead of assuming that the user doesn't want dependencies.yaml just because they forgot to include it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other things about this project would need to change to support sdists too.

The relative paths that reach up above pyproject.toml, like this:

[tool.rapids-build-backend]
dependencies-file = "../../dependencies.yaml"

are unlikely to work in an sdist, where I'd expect pyproject.toml to be at the root of the file layout.

This is just another place that's exposing the tension between building from an sdist vs. the file layout in source control (#17 (comment)).

From my perspective, having rapids-build-backend be stricter here (raising an error if the file that dependencies-file points to does not exist) is useful at this stage of development, where we're only building wheels and don't know the exact shape of what building RAPIDS sdists would even look like (#17 (comment)).

But if we don't want to do that, then I think raising this warning is sufficient (to at least help with debugging mistakes like the one I made in cudf) and that we shouldn't try to add configuration to differentiate between the sdist vs misconfiguration distinction. I think any attempt to do that is going to be difficult to review without knowing what we want out of sdist support, and that sdist support isn't a high priority right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I raised the sdist discussions, but I am also leaning towards us not needing to plan for them right now. I think it's causing additional complexity that isn't merited at this stage. I'd be fine with being stricter now and loosening up restrictions if and when we decide that we want to support sdists. The current state is a bit confusing because the backend does wrap the wrapped backend's build_sdist call correctly right now, which makes it seem like we support sdists, but I don't want to remove that stuff. For now we can leave it in whatever half-supported state is convenient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks for that!

I think we should proceed right now with adding this warning, and defer any other changes related to this.

I spent an hour debugging issues in my cudf PR that had the root cause "misconfigured dependencies-file". Having this case be an error would have saved me 59 of those 60 minutes... having this result in a warning would have saved me 58 of those 60 minutes. So the warning gets most of the benefit I wanted out of this discussion, and leaves the library in a state where it's possible to experiment with sdists.

@KyleFromNVIDIA I just updated this to latest main now that you merged #30 and #32 . I think we should merge this and then cut a release.

try:
parsed_config = rapids_dependency_file_generator.load_config_from_file(
config.dependencies_file
)
except FileNotFoundError:
msg = (
f"File not found: '{config.dependencies_file}'. If you want "
"rapids-build-backend to consider dependencies from a dependencies file, ",
"supply an existing file via config setting 'dependencies-file'.",
)
warnings.warn(msg, stacklevel=2)
parsed_config = None

try:
Expand Down