issue_comments

Thanks @marscher This is a great change.

I see this is your first PR. Welcome to Xarray!

Thanks for the warm welcome.

Thanks @marscher This is a great change.

I see this is your first PR. Welcome to Xarray!

Unit Test Results

0 files  0 suites   0s :stopwatch: 0 tests 0 :heavy_check_mark: 0 :zzz: 0 :x:

Results for commit 28c84858.

:recycle: This comment has been updated with latest results.

Hello @marscher! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-10-29 17:39:51 UTC

Test failure is real xarray/__init__.py:34: error: Name "_version" already defined (possibly by an import) [no-redef] Found 1 error in 1 file (checked 145 source files)

I think we just revert the last commit and merge...

All suggestions of yours are now included and CI is green. Thank you all very much for your review and suggestions.

thanks for the PR, @marscher, and especially for being patient with us/me.

if the CI still passes without the explicit mention of setuptools in the CI environments this should be ready for merging. It would probably be good to review the change to the dependency policy, though (do we need to list setuptools even if we only depend on it at build time?)

the CI passes, too. Now we just need an entry in whats-new.rst saying that the runtime dependency on setuptools was dropped, and we might also want to remove the explicit mention of setuptools in the CI files

I installed the precommit hook and everything passed locally.

I wonder if there are any tests for the backend entry points or if tests are just being skipped if the mandatory backends are unavailble (due to a bug introduced by switching from pkg_resources). I have like 5600 skipped tests when I naively invoke pytest.

thanks, @dopplershift. We agreed on doing that in the last meeting, and to then drop the new dependency together with python=3.7 in December. That means that if we can figure out the remaining issues we can include this in the next release (which will be very soon!)

You cannot use selectors with noarch conda packages. Full stop.

For conda-forge, it's perfectly fine to just unconditionally depend on the importlib_metadata backport, especially with the implementation done like here where you use the std lib version and fall back.

If one installs local packages with conda, all dependencies are ignored (--use-local flag). I have not yet figured out, why this is the case due to a lack of time.

I might be confusing something, but I thought preprocessing selectors were executed at build time? So if you build the noarch: python package in an environment that has python=3.7 you will keep the importlib_metadata while in a different one with python=3.8 it will be removed?

The easiest way to figure this out would be to build in a py38 environment, install in a py37 environment and then try to import it (i.e. trigger the code path that uses importlib.metadata). If that works, then it would be fine to merge now.

I don't think that noarch packages are restricted to python version selectors any more. I've tested this locally with a dummy recipe like this: ```yaml package: name: {{ name|lower }} version: {{ version }}

source: path: .

build: noarch: python number: 0 script: "{{ PYTHON }} -m pip install . -vv"

requirements: host: - python - pip run: - python - importlib_metadata # [ py < 38] ``` This creates a noarch package as expected with a recent conda/build version.

This means you can merge this and still use a noarch package on Conda-forge, which will conditionally install the backport importlib-metadata package for older Python versions.

is there any reason not to merge this?

the issue is that with conda we can either always install importlib_metadata or not at all: conda doesn't support noarch_python packages with preprocessing selectors (see the comments around https://github.com/pydata/xarray/issues/4295#issuecomment-667657310 for more discussion).

importlib.metadata is included in python>=3.8, which we can officially require from end of December this year.

All of this means that if we want to merge this now we have to decide between not releasing until we can drop python=3.7 or always installing importlib_metadata (which is what the pint feedstock does). However, I guess the cleanest way would be to wait until we can require python>=3.8.

Thanks for the PR @marscher

I don't know this area well — assuming this cuts the import time down considerably, is there any reason not to merge this?