-
-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Installing praw from source or sdist fails due to importing runtime deps at build time #2004
Comments
When I run your reproductions steps I'm not encountering this issue with Here's what I'm doing:
|
Sorry, I copy-pasted a lot of the relevant content from the prawcore issue, and I missed updating some important bits. Namely, the error message will reference whatever dependenc(ies) of the package don't already happen to be installed in the build environment, starting from the first missing import it hits, in this case After you manually install So, TL;DR, the issue is still present as reported, just with a different error message, harder to hack around and more likely to occur (due to the dependencies being much less common to happen to be installed in the build environment due to other build deps or by default). I've updated the title and initial description accordingly, thanks. |
This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days. |
Hey @LilSpazJoekp , have you gotten a chance to give this another look and lmk how you want to move forward here? Simply using static metadata and having the helper script you already use to update the version other places updated it too is, after a lot of careful thought and testing is the solution I'd recommend, as it is a ≈2 line change that doesn't require touching the code at all and is fully backward-compatible, plus gets you the benefits of standardized static metadata and can be easily applied consistently across all your repos without requiring customization and testing for each. However, I've also proposed another solution along the lines of what I originally implemented in Lmk, thanks! |
This issue is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days. |
This issue was closed because it has been stale for 30 days with no activity. |
Hey, so seems this was automatically closed while it was still waiting for maintainer input from @LilSpazJoekp . In any case, Conda-Forge builds (and quite probably other downstreams and builds) of PRAW 7.8.0 and above are now activately broken due to this issue (see e.g. conda-forge/praw-feedstock#26 ) and the issue is now urgent. The hacky temporary workaround for prawcore, manually installing all the runtime dependencies at build time (a single extra package, requests, also required for the backend anyway) doesn't really scale well to PRAW given it has many dependencies with different specs. The simplest and overall best path forward, that would be easy for me (or you) to implement on all 4 praw/prawcore/async projects with no additional changes or user-facing impact, would be to just specify the version as static metadata in Alternatively, we could implement the other solution of refactoring |
Sorry this slipped through. I'm good with your recommendations. Feel free to move forward those PRs and I can get them merged in. Again, apologies for this slipping through my life has gotten way busier since my twins were born. |
Describe the Bug
Same issue as praw-dev/prawcore#164 and @LilSpazJoekp requested followup from praw-dev/prawcore#165
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 fromconst.py
, this requires the runtime dependencies 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 eitherflit_core
orpraw
's dependencies were to ever change, this would break isolated builds too. Andrequests
isn't an exposed non-backend dependency offlit-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
praw
in any other context thanpip
'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 newprawcore
2.4.0 version for Conda-Forge—you can see the full Azure build log.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
Praw 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".While there are other ways to achieve this, as described in praw-dev/prawcore#164 , @LilSpazJoekp indicated he preferred moving
__version__
to be in__init__.py
directly, so Flit can find it by static inspection without triggering an import, and naturally asked for the same approach here as applied in praw-dev/prawcore#165 .This works, with one additional complication: the
USER_AGENT_FORMAT
module-level constant inconst.py
relies in turn on__version__
, which means thatpraw.const
needs to import it frompraw
(__init__
), and becausepraw.reddit.Reddit
is imported in__init__
for convenience (which in turn importspraw.const
), this creates an import cycle (as well as makingpraw
expensive to import, on the order of multiple seconds on an older machine).This cannot be trivially solved by a scope-local import, because
USER_AGENT_FORMAT
is a module-level constant. The simplest and likely least disruptive solution, sinceUSER_AGENT_FORMAT
is only used on place in PRAW (praw.reddit
) is to just inject__version__
at the one point of use along with the user-configured user agent rather than statically, eliminating an extra layer of format replacement nesting in the process.The one non-trivial impact from this is requiring a change to user code directly using
USER_AGENT_FORMAT
themselves to insertpraw.__version__
along with their own user agent. After various grep.app queries, which scans the entirety of public code on GitHub, I did find a single instance of this that would be affected, in the (closed-development) nnreddit project providing a Reddit backend for the Gnus reader. However, the fix is straightforward (an extra arg to oneformat
call) and backward-compatible with older PRAW, and the primary change here, removing,__version__
frompraw.const
is also equally technically backward-incompatible and was accepted inprawcore
, so not sure how much of an issue that is for you.Using a named placeholder for the PRAW version, e.g.
"{} PRAW/{praw_version}"
, would give a clearer error message as to what is needed to add to theformat
call, while modifying existing code (e.g.USER_AGENT_FORMAT.format(user_agent)
) to support the new PRAW version (i.e.USER_AGENT_FORMAT.format(user_agent, praw_version=praw.__version__)
would be fully backward compatible withUSER_AGENT_FORMAT
in older PRAW, asformat
will simply drop arguments that don't match placeholders in the string.The other alternative as mentioned in the other issue would be simply setting the version statically in
pyproject.toml
. This would avoid all these issues and needing to change anything else besides theset_version.py
script as well as give you the benefits of static metadata. It would mean the version is defined multiple places which you wanted to avoid, but if you're using that script to set it anyway, it shouldn't actually require any additional maintainer effort since the script could take care of it too. Of course, if that's the case we'd presumably want to go back and make that same change inprawcore
too; however, it would simplify the implementation inasyncpraw
(as well asasyncprawcore
) and avoid this same issue withUSER_AGENT_FORMAT
, and for whichasyncpraw.const.__version__
is used at least once in the wild.Up to you!
Code to reproduce the bug
In a fresh venv without requests or praw installed, e.g.
python3 -m venv .venv source .venv/bin/activate pip install --upgrade pip wheel
Run:
My code does not include sensitive credentials
Relevant Logs
This code has previously worked as intended
Yes
Operating System/Environment
Any
Python Version
Any, tested 3.9-3.12
PRAW Version
7.7.2.dev0
Links, references, and/or additional comments?
No response
The text was updated successfully, but these errors were encountered: