Copilot commented on code in PR #10344:
URL: https://github.com/apache/gravitino/pull/10344#discussion_r2912330536
##########
clients/client-python/setup.py:
##########
@@ -38,7 +38,7 @@
url="https://github.com/apache/gravitino";,
python_requires=">=3.10",
keywords="Data, AI, metadata, catalog",
- packages=find_packages(exclude=["tests*", "scripts*"]),
+ packages=find_namespace_packages(exclude=["tests*", "scripts*"]),
Review Comment:
There’s no automated coverage ensuring the built artifact actually
contains/imports these namespace subpackages. Current Gradle test tasks run
with `PYTHONPATH=clients/client-python`
(clients/client-python/build.gradle.kts:211-237), so packaging regressions
won’t be caught. Please add a CI/build check that builds the sdist (and ideally
a wheel) and performs an import smoke-test from an installed artifact (covering
at least modules under `gravitino.api.rel.partitions` /
`...expressions.distributions`).
##########
clients/client-python/setup.py:
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-from setuptools import find_packages, setup
+from setuptools import find_namespace_packages, setup
Review Comment:
PR description is still the template and doesn’t explain why packaging is
being changed (e.g., whether this is fixing missing modules in the published
sdist/wheel). Please update the PR description to state the concrete packaging
issue being addressed and how it was validated.
##########
clients/client-python/setup.py:
##########
@@ -38,7 +38,7 @@
url="https://github.com/apache/gravitino";,
python_requires=">=3.10",
keywords="Data, AI, metadata, catalog",
Review Comment:
Switching to `find_namespace_packages()` changes packaging semantics. This
is only necessary if the source tree intentionally contains PEP 420 namespace
subpackages (e.g., `gravitino/api/rel/partitions/` and
`gravitino/api/rel/expressions/distributions/` currently have no
`__init__.py`). If those missing `__init__.py` files are accidental, it would
be clearer to add them and keep `find_packages()`; otherwise, please add a
brief note here explaining that namespace packages are required so these
subpackages are included in the sdist/wheel.
```suggestion
keywords="Data, AI, metadata, catalog",
# Use find_namespace_packages because the Gravitino client uses PEP 420
# namespace packages (for example, gravitino.api.rel.partitions and
# gravitino.api.rel.expressions.distributions). This ensures these
# subpackages are included in the built sdist/wheel.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
