Installing from prawcore sdist fails without requests in build environment

CAM-Gerlach commented 11 months ago

Describe the Bug

Due to the use of Flit's fallback automatic metadata version extraction needing to dynamically import the package __init__.py instead of reading it statically (see pypa/flit#386 ) , since the actual __version__ is imported from const.py, this requires requests be installed in the build environment when building or installing from an sdist (or the source tree).

This happens merely by chance to currently be a transitive backend dependency of flit_core and thus building in isolated mode works. However, this shouldn't be relied upon, as if either flit_core or prawcore's dependencies were to ever change, this would break isolated builds too. And requests isn't an exposed non-backend dependency of flit-core, so it doesn't actually get installed otherwise.

This means Requests must be manually installed (not just the explicitly pyproject,toml-specified build dependencies) if building/installing prawcore in any other context than pip's isolated mode, e.g. without the --no-build-isolation flag, via the legacy non-PEP 517 builder, or via other tools or most downstream ecosystems that use other build isolation mechanisms. In particular, this was an issue on conda-forge/prawcore-feedstock#14 where I was packaging the new prawcore 2.4.0 version for Conda-Forge—you can see the full Azure build log.

Of course, this can be worked around for now by manually installing requests into the build environment, but that's certainly not an ideal solution as it adds an extra build dependency (and its dependencies in turn), requires extra work by everyone installing from source (without build isolation), makes builds take longer, and is fragile and not easily maintainable/scalable long term for other runtime dependencies.

Desired Result

Prawcore is able to be built and installed without installing requests, or relying on it happening to be a transitive build backend dependency of flit_core and present "by accident".

There are several ways to achieve this, all potentially reasonable options:

Move __version__ to be in __init__.py directly, so Flit can find it by static inspection without triggering an import
Include version as static metadata in the pyproject.toml instead of relying on dynamic backend-specific detection
Refactor __init__.py to be as minimal as possible and avoid inefficiently importing your entire package (or only do so lazily), as generally recommended.

Code to reproduce the bug

# In a fresh venv without requests or prawcore installed
pip install flit-core  # Or flit
pip install --no-build-isolation prawcore --no-binary prawcore

The `Reddit()` initialization in my code example does not include the following parameters to prevent credential leakage:

client_secret, password, or refresh_token.

[X] Yes

Relevant Logs

### Logs from local pip install of sdist ###

$ pip install --no-build-isolation prawcore --no-binary prawcore
DEPRECATION: --no-binary currently disables reading from the cache of locally built wheels. In the future --no-binary will not influence the wheel cache. pip 23.1 will enforce this behaviour change. A possible replacement is to use the --no-cache-dir option. You can use the flag --use-feature=no-binary-enable-wheel-cache to test the upcoming behaviour. Discussion can be found at https://github.com/pypa/pip/issues/11453
Collecting prawcore
  Using cached prawcore-2.4.0.tar.gz (15 kB)
  Preparing metadata (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Preparing metadata (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [22 lines of output]
      Traceback (most recent call last):
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 353, in <module>
          main()
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 335, in main
          json_out['return_val'] = hook(**hook_input['kwargs'])
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 149, in prepare_metadata_for_build_wheel
          return hook(metadata_directory, config_settings)
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\flit_core\buildapi.py", line 49, in prepare_metadata_for_build_wheel
          metadata = make_metadata(module, ini_info)
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\flit_core\common.py", line 425, in make_metadata
          md_dict.update(get_info_from_module(module, ini_info.dynamic_metadata))
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\flit_core\common.py", line 222, in get_info_from_module
          docstring, version = get_docstring_and_version_via_import(target)
        File "C:\Miniconda3\envs\qt6-env\lib\site-packages\flit_core\common.py", line 195, in get_docstring_and_version_via_import
          spec.loader.exec_module(m)
        File "<frozen importlib._bootstrap_external>", line 883, in exec_module
        File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
        File "C:\Users\C. A. M. Gerlach\AppData\Local\Temp\pip-install-1uivtksa\prawcore_cee755c9a2dc47b29dfbe46b846de281\prawcore\__init__.py", line 5, in <module>
          from .auth import (
        File "C:\Users\C. A. M. Gerlach\AppData\Local\Temp\pip-install-1uivtksa\prawcore_cee755c9a2dc47b29dfbe46b846de281\prawcore\auth.py", line 8, in <module>
          from requests import Request
      ModuleNotFoundError: No module named 'requests'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

This code has previously worked as intended.

Yes

Operating System/Environment

Any, tested Linux, Windows

Python Version

Any; tested 3.10, 3.12

prawcore Version

2.4.0 (newely introduced due to switch to Flit)

Anything else?

Just for reference, this was the build environment on Azure:

    _libgcc_mutex:    0.1-conda_forge           conda-forge
    _openmp_mutex:    4.5-2_gnu                 conda-forge
    bzip2:            1.0.8-h7f98852_4          conda-forge
    ca-certificates:  2023.7.22-hbcca054_0      conda-forge
    flit-core:        3.9.0-pyhd8ed1ab_0        conda-forge
    ld_impl_linux-64: 2.40-h41732ed_0           conda-forge
    libexpat:         2.5.0-hcb278e6_1          conda-forge
    libffi:           3.4.2-h7f98852_5          conda-forge
    libgcc-ng:        13.2.0-h807b86a_2         conda-forge
    libgomp:          13.2.0-h807b86a_2         conda-forge
    libnsl:           2.0.1-hd590300_0          conda-forge
    libsqlite:        3.43.2-h2797004_0         conda-forge
    libuuid:          2.38.1-h0b41bf4_0         conda-forge
    libzlib:          1.2.13-hd590300_5         conda-forge
    ncurses:          6.4-hcb278e6_0            conda-forge
    openssl:          3.1.3-hd590300_0          conda-forge
    pip:              23.3-pyhd8ed1ab_0         conda-forge
    python:           3.12.0-hab00c5b_0_cpython conda-forge
    readline:         8.2-h8228510_1            conda-forge
    setuptools:       68.2.2-pyhd8ed1ab_0       conda-forge
    tk:               8.6.13-h2797004_0         conda-forge
    tzdata:           2023c-h71feb2d_0          conda-forge
    wheel:            0.41.2-pyhd8ed1ab_0       conda-forge
    xz:               5.2.6-h166bdaf_0          conda-forge

And this was the relevant portion of the build log:

### Logs from Azure build of Conda-Forge package ###

# https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=806405&view=logs&jobId=656edd35-690f-5c53-9ba3-09c10d0bea97&j=656edd35-690f-5c53-9ba3-09c10d0bea97&t=986b1512-c876-5f92-0d81-ba851554a0a3

Source cache directory is: /home/conda/feedstock_root/build_artifacts/src_cache
Downloading source to cache: prawcore-2.4.0_b7b2b5a1d0.tar.gz
Downloading https://pypi.io/packages/source/p/prawcore/prawcore-2.4.0.tar.gz
INFO:conda_build.source:Success
Success
Extracting download
source tree in: /home/conda/feedstock_root/build_artifacts/prawcore_1697605563769/work
export PREFIX=/home/conda/feedstock_root/build_artifacts/prawcore_1697605563769/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl
export BUILD_PREFIX=/home/conda/feedstock_root/build_artifacts/prawcore_1697605563769/_build_env
export SRC_DIR=/home/conda/feedstock_root/build_artifacts/prawcore_1697605563769/work
Using pip 23.3 from $PREFIX/lib/python3.12/site-packages/pip (python 3.12)
Non-user install because user site-packages disabled
Ignoring indexes: https://pypi.org/simple
Created temporary directory: /tmp/pip-build-tracker-h18miru8
Initialized build tracking at /tmp/pip-build-tracker-h18miru8
Created build tracker: /tmp/pip-build-tracker-h18miru8
Entered build tracker: /tmp/pip-build-tracker-h18miru8
Created temporary directory: /tmp/pip-install-qe_g7qir
Created temporary directory: /tmp/pip-ephem-wheel-cache-gmhw8jmc
Processing $SRC_DIR
  Added file://$SRC_DIR to build tracker '/tmp/pip-build-tracker-h18miru8'
  Created temporary directory: /tmp/pip-modern-metadata-h0yhew9i
  Preparing metadata (pyproject.toml): started
  Running command Preparing metadata (pyproject.toml)
  Traceback (most recent call last):
    File "/home/conda/feedstock_root/build_artifacts/prawcore_1697605563769/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
      main()
    File "/home/conda/feedstock_root/build_artifacts/prawcore_1697605563769/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
                              ^^^^^^^^^^^^^^^^^^
  File "$PREFIX/lib/python3.12/site-packages/pip/_internal/operations/build/metadata.py", line 37, in generate_metadata
    raise MetadataGenerationFailed(package_details=details) from error
pip._internal.exceptions.MetadataGenerationFailed: metadata generation failed

LilSpazJoekp commented 11 months ago

Refactor __init__.py to be as minimal as possible and avoid inefficiently importing your entire package (or only do so lazily), as generally recommended.

What would this look like? I'm hesitant to do the other two options because we have tooling that increments __version__ in the const.py file. Option 1 could break any downstream dependencies that rely on __version__ being in the const.py module (though this is unlikely to be the case, but still possible). Option 2 is not feasible as we've intentionally avoided defining the package's version in multiple spots to simplify releases.

CAM-Gerlach commented 11 months ago

Refactor init.py to be as minimal as possible and avoid inefficiently importing your entire package (or only do so lazily), as generally recommended.

What would this look like?

It would be by a large margin the most involved of the proposed changes; I almost didn't include it at all, other than to point out that solving the bigger issue of the __init__.py (eager-)importing most of the rest of the package would also solve this issue. The "simple" approach would involve simply eliminating these imports, but that would of course break any client code relying on these conveniences rather than importing them directly from the relevant submodules. To avoid this, you could retain them but have them lazy-loaded, so the imports are only actually performed if the relevant attributes are accessed, using, for example, the SPEC 0001 approach.

Of course, this as mentioned this is likely a far too involved solution just to solve this problem, as opposed to the much more general one of eager-loading your various submodules in the __init__.py, so I assume you'll want to focus on the other two proposed solutions.

I'm hesitant to do the other two options because we have tooling that increments version in the const.py file.

Could you not simply replace const.py with __init__.py in your custom tools/set_version.py script? As far as I can tell, it should otherwise work exactly the same.

Option 1 could break any downstream dependencies that rely on version being in the const.py module (though this is unlikely to be the case, but still possible).

As you say seems very unlikely that users would be importing it from praw.const instead of the more obvious and much more conventional top-level praw (and there are few good third-party programmatic use cases for __version__ to begin with—I mostly find it useful interactively, e.g. for debugging).

We can actually quantify this, though, using e.g. grep.app which searches all of GitHub, which finds:

Search	`praw`	`praw.const`	`prawcore`
`import X`	485 hits	[0 hits]()	73 hits
`from X import`	68 hits	3 hits	46 hits
`from X import Y`	N/A	0 hits	N/A
`X.__version__`	4 hits	0 hits	1 hit
`from X import .*__version__`	2 hits	0 hits	0 hits

So, as far as public code on GitHub goes prawcore.const isn't even imported once (and even praw.const is only imported 3 times total, two of them in PRAW itself, none for __version__), never mind is __version__ used from it, and even the package-level __version__ is only used a couple times. Therefore, it seems fairly safe to conclude that it is rather unlike that even one user is importing it from __version__, which is substantially fewer users than who may be impacted now and in the future by this missing dependency issue.

Nevertheless, we could resolve even this small chance by importing __version__ in const.py from the top-level prawcore package—unless that resulted in an import cycle, which if so is another reason why eager-importing everything in __init__ is generally not usually a great idea.

Option 2 is not feasible as we've intentionally avoided defining the package's version in multiple spots to simplify releases.

That's true, though there are non-trivial tooling and ecosystem benefits to declaring it as static metadata, and I assume you could just tweak your version bump script to update the version there too—though I understand the desire to single-source it, of course.

Overall, Option 1 seems by far the least net disruptive. I could submit a PR for this, if you'd prefer. Thanks!

LilSpazJoekp commented 11 months ago

Makes sense to me.

Overall, Option 1 seems by far the least net disruptive. I could submit a PR for this, if you'd prefer. Thanks!

If you'd like, please do!

CAM-Gerlach commented 11 months ago

Thanks! Opened as PR #165

CAM-Gerlach commented 9 months ago

Thanks for circling back and closing this out! I opened praw-dev/praw#2004 to cover this in PRAW itself (and we in turn might want to reconsider Approach 2 here, if that's what's desired over there and in asyncpraw, etc).

praw-dev / prawcore