Repository navigation
Switch to "new_build_ext" broke PyYAML #4568
Description
@scoder @matusvalo This change killed some nasty dynamic build_ext subclassing that PyYAML's been doing since the dawn of time (see yaml/pyyaml#601 and https://github.com/yaml/pyyaml/blob/6.0/setup.py#L174-L238). The most immediate issue is just that the cython_sources method from old_build_ext that PyYAML's subclass was calling is no longer available- I tried a few different ways of patching it on with no success (various issues around missing instance data on either the command or the extension by the time get_source_files is called).
A couple of questions:
- Would you consider a reasonably simple change to emulate the
cython_sourcesmethod fromold_build_ext(to keep already-released PyYAMLbuild_extsubclasses working on 3.0.0a10+ without modification)? That's assuming I can come up with one- so far, that's not looking good... - Curious what the long-term plan is for
old_build_ext? I can make things work again in future PyYAML releases without rewiring all the ancient build code just by swapping inold_build_ext(eg WIP keep PyYAML build working on Cython3.0.0a10+ yaml/pyyaml#602), but not sure how long it'll be around.
Thanks!
(I can open an issue for this, but if the answers to the above are "no" and "not very long", not much point 😉 )
Originally posted by @nitzmahone in #4498 (comment)
Activity
scoder commentedon Jan 13, 2022
Thanks for reporting this. The way that function is being used in PyYAML seems to intend replacing the .pyx sources by the generated .c sources. If that's it, then calling
cythonize()instead might work. Not even sure if you need to inherit from Cython'sbuild_extin that case at all (or could simply use the one from setuptools).If it's just about calling
cythonize(), then it's probably ok for us to re-add that to keep it working.It's an annoying piece of outdated and duplicated code, not very well tested, and thus likely to break silently at some point. In terms of project management, that probably means that it will stick around forever, but not necessarily in a usable state for very long.
nitzmahone commentedon Jan 14, 2022
Thanks for the response and thoughts. I did mess around with just calling
cythonizefrom acython_sourcesmethod added to the newbuild_extbut ran into a bunch of other failures that look like the extension we're passing in hasn't been filled out enough yet. I'm guessing it's related to one or both of the pyyaml setup code also subclassingExtensionand that the new Cythonbuild_extdoesn't use all the same hook points that the old one did.I'll keep playing with it- I'm hoping to find a simple and acceptable change that'll keep the existing releases working on Cython 3.0, since we recently made Cython a hard build dep in pyproject but didn't pin a version in there. Worst case, I can just do patch releases on those branches and pin the pyproject build dep to
Cython<3.0, then solve the problem "properly" for new releases.ferdnyc commentedon Jun 24, 2023
@nitzmahone
Unfortunately, this only solves the problem as far forward as Python 3.12.x, but not for the (in-development) Python 3.13.
Cython<3.0has a dependency on the deprecatedcgimodule (imported byCython.Tempita._tempita), which was removed from the stdlib ~ a month ago. (See @vstinner's python/cpython@08d5923896.)So in Python 3.13 as of last month,
Cython==0.29.35fails with anything and everything due to the missingcgimodule, andCython>=3.0.0b1fails to buildPyYAML==6.0due to the unconditional use ofbuild_ext.cython_sources()in the latter'ssetup.py.matusvalo commentedon Jun 25, 2023
importing
cgimodule was removed in the following commit: 54b92e3. I think it will be fixed in Cython0.29.36release.ferdnyc commentedon Jun 25, 2023
@matusvalo
Looks like that SHA-1 somehow acquired a rogue 'z' on the end, but I managed to track down 54b92e3 (the merge of #5128).
I take it there will be an 0.29.36 release, then? That's good news, thanks. I saw that 3.0.0 is up to the RC stage, so I wasn't sure if the 0.29 series was still being updated.
matusvalo commentedon Jun 25, 2023
Ah snap! Fixed my comment.
I cannot tell you with 100% confidence because I am not deciding about releases, but I suppose there will be release since it was merged to 0.29.x branch. @scoder can provide you more accurate information about future releases...
ferdnyc commentedon Jun 25, 2023
@matusvalo
Ah, good point. And looking at the most recent entries in the #5449 history, I'd say this counts as a firm yes:
wbarnha commentedon Jun 28, 2023
Just wanted to add to the discussion, since this is the only place I can find ongoing discussion regarding 0.29.36 (even in the mailing list). I need 0.29.36 for PyPy support as noted in https://github.com/cython/cython/milestone/96?closed=1. I'm sure the maintainers are very busy. I'd be grateful if some clarification could be provided on the plan to release 0.29.36.
Sorry this isn't directly relevant to the bug itself and creating noise for all subscribers.
matusvalo commentedon Jul 7, 2023
@wbarnha Cython 0.29.36 was released.
fix: pin PyYAML to 5.3.1 for Python versions >= 3.10
fix: pin PyYAML to 5.3.1 for Python versions >= 3.10
Uzume commentedon Aug 1, 2023
PyYAML should switch to using
Cython.Build.build_ext(it will soon enough have to jettisondistutilswith Python 3.12 later this year and it might as well also jettisonCython.Distutils).One issue with this is, currently PyYAML intentionally makes Cython a build-time only dependency. At Distributing Cython modules Cython documents a method to accomplish this using
Cython.Build.cythonize(the documentation usesUSE_CYTHONinstead ofwith_cythonused in PyYAML). Cython also documentsCython.Build.build_extbut only as a hard dependency all the time. Can the Cython documentation be updated to document a build-time only dependency using theCython.Build.build_extmethod?Any maybe Migrating from Cython 0.29 to 3.0 can also mention how to port away from
Cython.Distutilsinterfaces.nitzmahone commentedon Aug 1, 2023
Yep, that's the plan- also why I didn't want to "sorta" add support for Cython 3 by switching to
old_build_ext- that's just giving us more excuses to defer fixing it properly.At least in PyYAML's case, the complexity is because PyYAML was actually further subclassing Cython's distutils stuff, and relying on a bunch of internal state and method call orders that differ greatly under
new_build_ext- I messed around for awhile ~18mo ago trying to get the distutils stuff working again under that, but IIRC it was going to require some pretty major hackery to get it to work (and again being very brittle/fragile and still relying on distutils anyway). Now that it's pretty stable and we've got better ways to pass config into the PEP517 backend, I'd much rather spend that time just making it work right sans distutils. 😆I'm not sure how many other projects are relying on the distutils stuff, but at least in our case, more docs wouldn't really help much. The build backend integration seems pretty straightforward, we just need to decide which of our own config knobs we want to preserve and implement support for whatever's needed before we hand back off to Cython's build backend, and decide if we want to do any further magic to better support the truly "Cython optional" cases it used to (eg, in-tree backend wrapper to allow native-only PEP517 builds sans Cython).
Uzume commentedon Aug 1, 2023
@nitzmahone: Since
distutilshas been a part ofsetuptoolsfor quite sometime now, PyYAML could consider piecemeal transitional changes. So long assetuptoolsis imported first,distutilsimported after that point should reliably exist (at least untilsetuptoolsstarts dumping stuff). PyYAML could incrementally:old_build_extcontinuing to usecython_sourcesnew_build_extandcythonize(in place ofcython_sources; the API is similar: filtering anExtensionsourcesvs. filtering anExtension)distoolsSomewhere along the way PyYAML may also want to avoid
new_build_extby using the stocksetuptools.command.build_extand directly usingcythonizesince that seems to have better documentation and may be simpler.I am not sure if that is all completely thought out since there are some dependencies but I think there may be a viable approach in there somewhere. And a total rewrite could be considered as well but without the urgency so it could be done right without rocking the boat too much.
szelenka commentedon Sep 6, 2023
@Uzume yaml/pyyaml#731 was closed in favor of this issue, but this issue seems to suggest there are code changes needed in the pyyaml project to make this work.
Should 731 be re-opened, or is there a different issue we can watch for when pyyaml will implement a fix?
Uzume commentedon Sep 6, 2023
@szelenka: moving forward pyyaml requires significant changes. The question is which changes and what approach. Although artifacts of this issue could perhaps be rectified in cython, I do not really think cython has done anything wrong and in fact is probably doing the right things in breaking pyyaml in this particular way. I do not think yaml/pyyaml#731 was closed in favor of this issue (but you would have to ask the closer @ahesford to be sure).
ahesford commentedon Sep 6, 2023
I didn't close yaml/pyyaml#731 "in favor of this issue", especially because this issue doesn't provide a fix for the problem solved by my patch. I closed the issue because it was apparent that political issues in PyYAML make incorporation of a trivial fix too costly. Rather than engage in further debate that was not likely to resolve the matter, I thought it best to abandon the effort and just incorporate the fix into the Void Linux package for PyYAML.
szelenka commentedon Sep 6, 2023
@ahesford does this imply that nobody from the PyYAML project is looking into a fix anymore?
ahesford commentedon Sep 6, 2023
I'm not part of the PyYAML project and don't know if anybody is looking for a fix. My opinion on this issue is that this is not Cython's problem to fix.
nitzmahone commentedon Sep 6, 2023
PyYAML "reluctant maintainer" here (my question that caused this issue to be opened is quoted above) - TL;DR: I have no problem with this issue being closed.
Sorta- I'm no longer considering releasing any "quick fixes" to make the existing creaky PyYAML build work under both Cython < 3 and 3+. I'd been monitoring the progress on Cython 3.0 every few weeks, but the rapid progression from betas to a final release caught me by surprise (stupid long vacations 😆), so I had to scramble to release the contingency plan that I'd hoped to have out around the time of Cython 3's RC (assuming I hadn't come up with a better solution). Since we had to do that, I'm sticking with not supporting Cython 3 until our build is rewritten.
A number of PyYAML's ancient build requirements/expectations (as well as some new ones) mostly preclude vanilla usage of
build_ext, at least without doing a lot of custom stuff up front, so our likely solution to proper Cython 3 support will be an in-tree backend wrapper over setuptools that makes the Cython dep truly optional based on customconfig_settings, then $handwavey_things to pass config tocythonizeand/orbuild_extin a way that allows all the necessary customizations. I've been picking at it for awhile, and think I have a pattern that more-or-less works and will hopefully be reasonably maintainable.