https://pantsbuild.org/ logo
Join Slack
Powered by
Oof. Before Poetry 1.2, it does not include `setup...
# development
h
Oof. Before Poetry 1.2, it does not include 
setuptools
and 
wheel
in lockfiles. We need those included. But they're currently at 1.2.0a2, and it took 3.5 months for 1.1 to go from a2 to stable release Some options:
1. Use unstable release of Poetry in production.. 2. Fork Poetry to make a one-line change that fixes this? I think #2 makes most sense, but I don't know how we handle users wanting to change to different versions of Poetry. Maybe we set the default to a Git fork, and then override the help message to caution that changing the version will require using 1.2+ or a forked version?
w
i think that given that our use of poetry is 1) internal, 2) for a brand new feature, using an unstable release is ok.
a
Yeah, it’s for a new and expeirmental feature. If Poetry releasing a stable release is the only blocker on this becoming non-experimental, that seems fine to me.
h
I at first was thinking that it's okay to use their alpha release, but it seems very unstable unfortunately 😕 For example, their alpha fails to generate a lockfile for our default 
pytest
due to 
Invalid PEP 440 version: '3.5.'
, even though 1.1 handles it great. Looks like a bug Even tho it's an implementation detail that we use Poetry, I think users will be frustrated if Pants's lockfile generation is buggy due to issues like this
a
hmm.
h
So, I think next best option is a fork against 1.1.7, w/ that one line config change? Not ideal, but seems preferable to buggy lockfile generation
a
I understand there’s a reason why they mark 
pip
and 
setuptools
as unsafe, when you’re actually using poetry for dependency management… Does that issue affect our use case?
h
(Another wildcard is for us to distribute Poetry as a pre-built PEX. They're no longer going to be doing GitHub releases for it w/ a pre-built binary, and you can only install now via 
pip
and friends like 
pipx
. But then we don't have a lockfile for Poetry itself, and I think it's a chicken and egg problem to have Pants use Poetry to generate a lockfile for Poetry That would give us deterministic installs and make it easy to patch this. But now it's harder for users to change the version used in the feature)
a
(disclosure: I think a fork is OK, but it’s probably best we keep the feature as experimental/only partially supported until Poetry supports it properly)
h
Does that issue affect our use case?
I don't think so. For any projects not using PEP 517, we need to have 
setuptools
included. In fact, this is one of our most common issues users ask for help w/ in Pants Slack. They have a 3rdparty dep that doesn't use PEP 517 and doesn't declare a dep on 
setuptools
, so they have to manually add the dependency. We need to preserve that explicit dep in the lockfile
a
I was wondering specifically whether the reason why poetry skips setuptools in 1.1 is likely to break our use of poetry if we don’t skip it
h
No, I don't think it will cause issues. If my read of https://github.com/python-poetry/poetry/issues/1584 is correct, it was originally because Poetry did not vendor setuptools et al? Doesn't look to be relevant anymore, and even if it was, we only do 
poetry lock
, rather than 
poetry install
Fwict, only downside is we now have to maintain a fork
a
IN which case, maybe we make this experimental (with a fork maintained) until Poetry 1.2 comes out, and then we do the work to make sure Poetry 1.2 works before doing a final release?
h
I really want to land lockfiles in 2.7 (at least tool lockfiles). It's bad we're not pinning tool installations right now. So I want to have a fully stable, polished experience by end of the month hopefully I think best path forward is fork 1.1.7 w/ the one line change and verify it works fine, and then use the options system so that users can upgrade to Poetry 1.2+ in the future even if using Pants 2.7 a year from now
Still the wildcard of whether we should distribute Poetry as a pre-built PEX to make its own installation deterministic and workaround the chicken and egg problem. But that's a separate, albeit related decision
Does that sound sensible? cc @witty-crayon-22786
w
if possible, i would defer the decision of whether to fork until the project is mostly done.
do we have workarounds for either bug that would defer doing this until other things are out of the way?
also, re: pre-built PEX: i don’t think there is a chicken and egg problem: poetry is only used to generate lockfiles. if you already have one, you can still build it. so afaict, no need for a PEX if you hand-make the lockfile (or have instructions for how to do it with poetry itself)
h
We don't have workarounds. If I use 1.2, pytest lockfile fails w/ that bug. If I use 1.1, we are missing 
setuptools
and 
wheel
from every lockfile, meaning 
setuptools_lockfile.txt
is empty 👀
i don’t think there is a chicken and egg problem: poetry is only used to generate lockfiles. if you already have one, you can still build it. so afaict, no need for a PEX if you hand-make the lockfile (or have instructions for how to do it with poetry itself)
Got it, so that mechanism will have to be a little differnt, to be clear? You manually provide the lockfile, rather than Pants running Poetry to generate it for you. Makes sense to me!
w
only if you want to use a different version… the same would go for generating a PEX
👍 1
the default version would be locked.
re: 1.2 + pytest: is the invalid version fixed in later versions of pytest…?
h
I haven't spent a ton of time debugging why that particular bug happens, it's something about it improperly converting a 
3.5.*
constraint to 
3.5.
, then PEP 440 erroring. (Not sure where 3.5.* even comes from) But my takeaway from it was: this is less stable than we want for our upcoming 2.7 release. If I figure out this particular bug, I'm sure we will have others
w
you have already found one bug in 1.1 as well, so it’s not like either of them is bug free. but yea, the discussion on that in https://github.com/python-poetry/poetry/issues/4176 is not reassuring.
is the 1.1 bug something that could be monkey-patched rather than forking for?
a
The unsafe packages list was defined inside a function
so it’s not as trivial as it could be
w
can patch the function
h
1.1's issue had been known for a long time. and it wasn't a bug, it was intentional iiuc. only we disagree w/ their intention
a
Oh, it’s a class attribute, not a function constant. +1 for patching in place if we can
w
but it’s changed/fixed in 1.2 …?
h
monkey patch how? change the PEX itself? seems risky if users depend on different versions than we expected - or we'd only patch if it's 1.1?
a
Drop some extra code into the pex, yes
w
only patch if it’s 1.1. use a script-entrypoint… something like 
-c 'import thing; <http://thing.to|thing.to>_change=fixed_definition; main()'
☝️ 2
🙌 1
h
It worked! Thanks for talking about this and the monkeypatch tip! FYI I had to use 
-m
and create a custom file loaded via 
--sources
. 
-c
looked for console scripts defined in the 3rd party deps
w
mm.. interesting. yea, i guess PEX mismatches 
python
for that flag
(tangent: … i think that if you don’t specify an entrypoint to PEX, the result is a file that just invokes the interpreter. and then you can pass 
-c
at runtime)
➕ 1
Open in Slack
PreviousNext
Powered by