Open jameslamb opened 5 months ago
@KyleFromNVIDIA is this closed by https://github.com/rapidsai/dependency-file-generator/pull/77? Or is there still more to do?
We still need to build the docs in CI and publish them to docs.rapids.ai.
Ah got it, ok. I didn't think publishing docs was part of the scope of this particular issue. Thanks for the explanation.
Details
The ongoing work on
rapids-build-backend
relies on usingrapids-dependency-file-generator
as an importable module, not a CLI, like thisref: https://github.com/rapidsai/rapids-build-backend/pull/17/files#r1542068869
Given that, I think some work should be done to make the boundary between public and private parts of the API here clearer.
Approach
__all__
entries in every module of the library*
imports__all__
entry in the top-level__init__.py
rapids_dependency_file_generator
module without needing to know about submodulesfrom rapids_dependency_file_generator import *
_
Benefits of this work
Clarifies the boundary between public and private, reducing the need for tight coordination between
rapids-build-backend
andrapids-dependency-file-generator
for some types of changes.Makes it easier for tools to help enforce those boundaries:
sphinx
autodoc will by default only auto-generate documentation for entries in__all__
(docs)_
(e.g.ruff
's pylintimport-private-name
check))Other notes
Consider the following.
only '__version__' is importable from the top-level namespace today (click me)
```shell pip install . python -c "from rapids_dependency_file_generator import *; print(dir())" ``` ```text ['__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__', '__version__'] ``` Only `__version__` is exported from the top-level module https://github.com/rapidsai/dependency-file-generator/blob/795c0c3afb55f6a11d36de7fa9b8f74001891327/src/rapids_dependency_file_generator/__init__.py#L3-L5 That means that things like `rapids-build-backend` that want to import other things have imported coupled to the exact file layout in this project, like this. ```python from rapids_dependency_file_generator.rapids_dependency_file_generator import ( get_requested_output_types, make_dependency_files, ) ``` In my opinion, it'd be better to re-export that stuff from the main module, so that `rapids-build-backend` could do this ```python from rapids_dependency_file_generator import ( get_request_output_types, make_dependency_files ) ``` And so we could freely re-arrange the modules inside this library without breaking `rapids-build-backend`. Exposing submodule import paths is really helpful in projects with large APIs like `sklearn` or `scipy`, but for this small library I think it'd be better to push downstream consumers to just import from the top-level namespace.submodules are re-exporting all their imports (click me)
Consider the following ```shell pip install . python -c "from rapids_dependency_file_generator.cli import *; print(dir())" ``` ```text ['Output', '__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__', 'argparse', 'default_dependency_file_path', 'delete_existing_files', 'generate_matrix', 'load_config_from_file', 'main', 'make_dependency_files', 'os', 'validate_args', 'version', 'warnings'] ``` It's technically possible right now to do something like this: ```python from rapids_dependency_file_generator.cli import argparse ``` That should be discouraged, via an `__all__` entry in `src/rapids_dependency_file_generator/cli.py` (and all the other submodules).