gh-108494: Argument Clinic partial supports of Limited C API #108495
Conversation
Argument Clinic now has a partial support of the Limited API: * Add --limited option to clinic.c. * Add '_testclinic_limited' extension which is built with the limited C API version 3.13. * For now, hardcode in clinic.py that "_testclinic_limited.c" targets the limited C API.
Adding _testclinic_limited.my_int_func() was made possible by the addition of the PyLong_AsInt() to the public C API yesterday 😇 Well, later we can consider adding support for limited C API version 3.12 and older by adding a different code path. But having PyLong_AsInt() helps to have a bare minimum starting point for this work. |
| @@ -2536,9 +2558,15 @@ def __repr__(self) -> str: | |||
| def parse_file( | |||
| filename: str, | |||
| *, | |||
| verify: bool = True, | |||
| output: str | None = None | |||
| ns: argparse.Namespace, | |||
There was a problem hiding this comment.
I'm replying here since I dislike comments lost on a commit, I don't think that many people will notice it.
I'm fine with people doing reviews after I merged changes (I like them!).
I don't like this at all :(
Requiring that an argparse.Namespace object be passed to parse_file() feels like we're coupling the parse_file() function to the specific context in which it's currently called. A Namespace object is essentially unstructured data; we should be parsing the Namespace object in run_clinic(), and passing only the specific variables we need to parse_file(). Apart from anything else, this makes the code much less type-safe.
Free feel to change it in a follow-up PR. Your rationale makes sense and I don't have a strong preference for passing arguments here. Or I can do it if you prefer.
I'm still struggling a lot with this code base and I tried to write the minimum working change without touching too many parts of the code. The problem is that I'm not used with the AC design, so I'm not sure if it's the right place to add code...
I'd appreciate if you'd give me a day or so to review Argument Clinic PRs @vstinner — I didn't see the review request for this one until it was already merged :)
I understand your frustration. I didn't expect any review at all in fact. I'm always happy to get reviews, even if it's after I merge my change.
Today I wanted to push multiple changes to unblock my work on removing private functions: see PR #108499 which kind-of the end of my "PR serie".
I'm frequently frustrated by the fact that GitHub doesn't let me create a "patch serie" (multiple PRs depending on each others). So I have to merge my changes one by one, but I'm always bitten by a CI which dislike my change, then I have to wait 30 min for the next issue... On the AC changes, I didn't expect that make check-c-globals would fail for example.
I wanted to push the basis on AC work to add #include and to support the limited C API. For following changes, it should easier to work on multiple PRs at the same time.
I'm glad that other people are now working on AC. Adding #include and supporting the limited C API was a long awaited feature for me, I started working on that in 2020 :-)
There was a problem hiding this comment.
I'm replying here since I dislike comments lost on a commit, I don't think that many people will notice it.
Oops, I didn't mean to add a comment on the commit — I meant to comment on the PR. Thanks!
There was a problem hiding this comment.
I understand your frustration. I didn't expect any review at all in fact.
Come on, you know that both Alex and I are active on a daily basis on CPython ;)
|
I wish this change had not been made in rush. First, the option should not be binary, it should specify the limited API version. |
| {parse_arguments})) | ||
| goto exit; | ||
| """, indent=4)] | ||
| argname_fmt = 'args[%d]' |
There was a problem hiding this comment.
I do not think that it is correct. args is a Python tuple object here.
There was a problem hiding this comment.
Oh ok, so I suppose that it can be removed.
I agree. Either we revert the rushed changes, or we try to fix these things according to established workflow next week. cc. @vstinner @AlexWaygood
That sounds like a better approach, indeed. |
|
See the related issue, we should be careful if we want to support older limited C API versions (really consider shipping binaries built with it and working on old Pytjon versions). There are many traps. The limited_capi type can be easily changed from bool and int. Since AC and argument parsing are complex and I am trying to get this feature since 2020, I propose to have an incremental approach, have tests and move step by step. Having a full support of any code path in AC for the limited C API would be a giant work, I expect that it would be a giant PR really hard to review. Sadly, I'm not sure that the limited C API is complete enough to support all cases. I added _testclinic_limited to check that the few formats already supported works as expected with functional tests. If you prefer a different development method and restart from scratch, sure, go ahead and revert my changes. |
Argument Clinic now has a partial support of the
Limited API: