UXARRAY / uxarray

Xarray-styled package for reading and directly operating on unstructured grid datasets following UGRID conventions
https://uxarray.readthedocs.io/
Apache License 2.0
142 stars 31 forks source link

Migrate to `pyproject.toml` and deprecate local `meta.yaml` #771

Closed philipc2 closed 2 months ago

philipc2 commented 3 months ago

Closes #772

Overview

philipc2 commented 2 months ago

This almost definitely isn't caused by the changes in this PR, but when I was testing the packaging locally, I noticed that you're including a lot of files in your packaging that you probably don't mean to.

By running python -m build and checking the generated uxarray.egg-info/SOURCES.txt, I can see you're including

.gitignore .pre-commit-config.yaml .github/ benchmarks/ ci/ docs/ test/ (not necessarily bad practice, but might be unintentional here, especially since you're packaging all the .nc files as well) We handle this in geocat-comp by having a MANIFEST.in, though there are other approaches.

This is a great suggestion, and thanks for guiding me how to find the SOURCES.txt. This is good to know!

I've added a MANIFEST.in file and the output looks like this now:

.pre-commit-config.yaml CONTRIBUTING.md INSTALLATION.md LICENSE MANIFEST.in README.md build.sh pyproject.toml setup.py uxarray/init.py uxarray/constants.py uxarray.egg-info/PKG-INFO uxarray.egg-info/SOURCES.txt uxarray.egg-info/dependency_links.txt uxarray.egg-info/requires.txt uxarray.egg-info/top_level.txt uxarray/conventions/init.py uxarray/conventions/descriptors.py uxarray/conventions/ugrid.py uxarray/core/init.py uxarray/core/api.py uxarray/core/dataarray.py ... the rest are uxarray .py modules

philipc2 commented 2 months ago

I am unable to import the package I build locally. Instead, I get a stacktrace with

ModuleNotFoundError: No module named 'holoviews' Details I imagine this is either because you need to add holoviews to your base dependencies or that the optional dependencies aren't working as you've intended.

I installed holoviews manually, repeated the process, and got the same error for datashader so I installed the rest of the viz requirements manually and then got

import uxarray as ux /Users/anissaz/miniconda3/envs/uxarray-build/lib/python3.10/site-packages/dask/dataframe/init.py:31: FutureWarning: Dask dataframe query planning is disabled because dask-expr is not installed.

You can install it with pip install dask[dataframe] or conda install dask. This will raise in a future version.

warnings.warn(msg, FutureWarning)

I've included the visualization dependencies as part of the required ones, since it makes more sense to bundle these considering that visualization is a major part of our feature set.

Now, the only optional dependencies are for Hongyu's math utility functions and for developers


[project.optional-dependencies]
complete = ["uxarray[math, dev]"]
math = ['gmpy2', 'pyfma']
dev = ['pathlib', 'pre_commit', 'pytest', 'pytest-cov', 'ruff']
philipc2 commented 2 months ago

I'm also seeing a couple warnings generated from my local build tests

Python recognizes 'uxarray.conventions' as an importable package[^1], but it is absent from setuptools' packages configuration. Puthon recognizes 'uxarray.conventions' as an importable package[^1], but it is absent from setuptools' packages configuration.

This leads to an ambiguous overall configuration. If you want to distribute this package, please make sure that 'uxarray.conventions' is explicitly added to the packages configuration field. Alternatively, you can also rely on setuptools' discovery methods (for example by using find_namespace_packages(...)/find_namespace: instead of find_packages(...)/find:).

You can read more about "package discovery" on setuptools documentation page: https://setuptools.pypa.io/en/latest/userguide/package_discovery.html

If you don't want 'uxarray.conventions' to be distributed and are already explicitly excluding 'uxarray.conventions' via find_namespace_packages(...)/find_namespace or find_packages(...)/find, you can try to use exclude_package_data, or include-package-data=False in combination with a more fine grained package-data configuration.

You can read more about "package data files" on setuptools documentation page: https://setuptools.pypa.io/en/latest/userguide/datafiles.html I'm also seeing similar warnings for uxarray.core, uxarray.grid, uxarray.io, uxarray.plot, uxarray.remap, uxarray.subset, and uxarray.utils.

I wonder if this is because throughout the code base, we are importing like this:

from uxarray.core import UxDataArray

As opposed to doing

from .core import UxDataArray

https://docs.python.org/3/reference/import.html#package-relative-imports

This thankfully seems like a warning, so I can turn this into an issue and investigate it further at a later time.

philipc2 commented 2 months ago

I believe it's because there is no 'matplotlib-base' pypi release, only on conda.

anissa111 commented 2 months ago

I believe it's because there is no 'matplotlib-base' pypi release, only on conda.

That makes sense. I looked at xarray, and it looks like they use matplotlib in their pyproject.toml and matplotlib-base in their feedstock meta.yaml

philipc2 commented 2 months ago

I've pulled the latest version, made a new fresh conda environment, built the package locally, installed it from the generated wheel, and successfully imported it in an ipython shell and tested the new versioning system. As far as I can tell, these changes are working as expected. I do want to note that I've never worked with optional dependencies and don't have a clear way to test that locally.

I would consider making a test upload to a personal channel and using that to test the optional dependency installs, but that might be overkill, so I'll leave that up to you.

I also suggest updating this:

https://github.com/UXARRAY/uxarray/blob/dff3971a67d7ff03f9af229ab1cd5feacf50537f/.pre-commit-config.yaml#L13

and this:

https://github.com/UXARRAY/uxarray/blob/dff3971a67d7ff03f9af229ab1cd5feacf50537f/.github/ISSUE_TEMPLATE/release_request.md?plain=1#L14

just to avoid future confusion, but those aren't anything technically "wrong", so I'll approve and you can decide what to do with those.

Thank you so much for the thorough review!