Additionally expose multidict as a Cython importable by Vizonex · Pull Request #1178 · aio-libs/multidict

Vizonex · 2025-06-17T21:13:13Z

What do these changes do?

These changes add a new C-API for Cython support with multidict. This is my second attempt at getting this passed but this time I've added everything in to ensure a smooth pull request.

What I will mainly need help with is figuring out are the following parts

MultiDictProxy Objects
istr objects (I think we need to make a new function for initalizing istr in cython for a bit of extra speed)
documentation (I haven't figured out how we should approach this one. )

Are there changes in behavior for the user?

Users could now import multidict as a cython library which is something not very commonly seen I also added a get_include function for finding the multidict C Headers, will have to start including them for now on so that users will be able to compile them to cython or other C/C++ Apis.

Related issue number

'fixes Implement C-API Capsule For Future CPython & Cython Support #1171' I was told that I had to wait for hash-tables to be implemented.

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes

webknjaz · 2025-06-18T07:04:22Z

FYI, we got rid of Cython in #249

asvetlov · 2025-06-18T09:25:04Z

FYI, we got rid of Cython in #249

@webknjaz it is for fast calls of multidict methods from Cython, not for using Cython for the multidict implementation

webknjaz · 2025-06-18T09:54:30Z

This is where you can see a preview of how this change note is rendered: https://multidict--1178.org.readthedocs.build/en/1178/changes/. Alternatively, if you want to build the docs locally, it'll also include draft/unreleased notes.

webknjaz · 2025-06-18T09:56:12Z

+    """Get multidict headers for compiling multidict with other
+    C Extensions or cython code"""


This doesn't seem PEP 257-compliant. Could you organize it as a summary line and a separate description?

Sure let me do so here in a bit I just woke up.

webknjaz · 2025-06-18T09:57:07Z

+    # NOTE: Import pathlib later so that were not being slow during
+    # other use-cases
+    import pathlib


I'm not sure if this is worth breaking the style for. Most people's apps would be importing pathlib anyway. So in real world, it'll likely be cached by the time multidict is imported.

webknjaz · 2025-06-18T10:00:15Z


 upstr = istr
+
+# Inspired by Numpy


Is this a comment for get_include()? Why is it several lines away from it? Code comments should be next to things they reference (ideally on the same line) so they don't drift even further away over time. Additionally, code comments should provide useful context that is otherwise unobvious, using complete sentenced (it is confusing to have 3 arbitrary words on a separate line without any other context that would at least seem related).
If you took this idea from elsewhere, it's of course good to give credit. But please also explain what the idea is in the first place.

webknjaz · 2025-06-18T10:02:18Z

+    # other use-cases
+    import pathlib
+
+    return str(pathlib.Path(__file__).parent)


Honestly, I'd avoid string-based path APIs. In Python, it's best to always use pathlib objects up until the point where strings are needed to interface with external systems. I think it's best to just return a pathlib object and let the caller convert it however they prefer. Maybe also .resolve() it before returning for a good measure.

The function returns a value that should be added to a configuration, e.g. as -I cmdline option.
It's not a path to operate on; it's always processed as a string.

.resolve makes sense, though.

I think it's not obvious to me (or probably @webknjaz) what the purpose of this function is? Is this some sort of standard interface to something?

It's not called anywhere in this PR and it's being exported as a public function. The docstring says it gets headers, but it appears to just return the path that multidict is installed in. Do people need this function, when they can just do multidict.__path__?

@Dreamsorcerer good question.

I guess it could be used as Extension argument for third-party C extensions:

setup(..., extensions=[Extension(..., include_dirs=multidict.get_include())])

The same could be applied to third-party Cython extensions

multidict.__path__ could do the same, probably. I have no strong preference.

Also, with the headers not being installed, is this unusable right now?

Also, with the headers not being installed, is this unusable right now?

I think I solved that by editing manifest.in recently.

That only governs what's included into sdists.

webknjaz · 2025-06-18T10:02:44Z

 {
    return PyModuleDef_Init(&multidict_module);
 }
+


accidental trailing line?

Suggested change

webknjaz · 2025-06-18T10:09:28Z

    return 0;
 }

+


Looks like other sections follow a single-line separator convention between function definitions, only using double-lines where those sections end.

Suggested change

Regardless, we shouldn't include unrelated formatting because it surfaces arbitrary chunks of code in the PR diff.

FYI, #1123 proposes plugging a code formatter for C. If you'd like to work on it, this would solve the problem of accidentally changing things in PRs. And it'd be useful to all of us in general.

webknjaz · 2025-06-18T10:10:20Z

 -r towncrier.txt
 -r doc.txt
 pyperformance==1.11.0
+cython==3.1.2


Why is this needed? Where is it used?

@webknjaz A little bit outdated but the plan was to use it for a cython test to ensure that the inner c-api to and from cython works correctly.

Then put it into ci.txt and add a code comment that it's for capsule testing.

Vizonex · 2025-06-19T17:07:06Z

I'm stumped on writing the workflow to compiling the _multidict_cython test module that I wrote so I will simply wait for help.

webknjaz

There's a lot of cleaning to be done before this is mergeable. I marked some places related to what we talked about on matrix.

webknjaz · 2025-06-19T20:53:59Z

 graft tests
 global-exclude *.pyc
 include multidict/*.c
+include multidict/_multilib/*.h


This adds the file to sdist. You may have to include it into wheels too.

I will make sure I do so. Thanks for reminding me about wheels. I will get that done after I write some new documentation for using multidict with cython.

webknjaz · 2025-06-19T20:59:34Z

+    # other use-cases
+    import pathlib
+
+    return str(pathlib.Path(__file__).parent)


That only governs what's included into sdists.

webknjaz · 2025-06-19T21:01:15Z

 -r towncrier.txt
 -r doc.txt
 pyperformance==1.11.0
+cython==3.1.2


Then put it into ci.txt and add a code comment that it's for capsule testing.

webknjaz · 2025-06-19T21:02:27Z

We should stop putting arbitrary artifacts into the test dir root. Ideally, they should just be generated and put into tmp dir. Or at least into a subdir.

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

…ution

for more information, see https://pre-commit.ci

Vizonex · 2025-06-20T16:08:20Z

Good news is that I did figure out that setuptools has no way to compile something without the setup portion so I came up with a workaround for it, cythonize was also pretty vague about this so that route turned out to just be a dead-end.

for more information, see https://pre-commit.ci

…tidict into Cython-Compatability

for more information, see https://pre-commit.ci

Vizonex · 2025-06-20T19:50:12Z

Moved to #1183 I will be using this information I got from this one to help me later.

Vizonex added 5 commits June 17, 2025 15:07

Add __init__.pxd

c26f06e

Create capsule.h

bac1e1d

Update _multidict.c

2f43fb6

Update __init__.py

3ddfc53

forgot to put get_include in __all__

35b7406

Vizonex requested a review from asvetlov as a code owner June 17, 2025 21:13

Vizonex added 2 commits June 17, 2025 16:15

Update __init__.py

86e55a7

Create 1178.feature.rst

7461cf7

Vizonex requested a review from webknjaz as a code owner June 17, 2025 21:18

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jun 17, 2025

Vizonex mentioned this pull request Jun 17, 2025

TypeError when using aiothreading.ThreadPool Vizonex/aiothreading#6

Closed

Vizonex added 6 commits June 17, 2025 21:13

run autopep8

8fa2b7f

Update __init__.py

b44fee9

run black

cc8ed9f

fix linting

dd09781

add cython

2e64455

Merge branch 'aio-libs:master' into Cython-Compatability

6af7486

webknjaz reviewed Jun 18, 2025

View reviewed changes

Comment thread CHANGES/1178.feature.rst Outdated

webknjaz reviewed Jun 18, 2025

View reviewed changes

webknjaz changed the title ~~Cython Extension for Multidict~~ Additionally expose multidict as a Cython importable Jun 18, 2025

webknjaz requested a review from bdraco June 18, 2025 10:13

Vizonex and others added 7 commits June 19, 2025 11:16

thanks asvetlov, I downloaded clang-format just to get this passed :)

fdb402e

clang-formatted _multidict.c

e6023f4

this should fix it

8451958

update capsule.h

c07080c

remember to compile cython test

51b0f3f

fix lint, cython already installs itself if I remember correctly

54351be

Maybe this will work???

9886334

Vizonex added 2 commits June 19, 2025 14:46

removing trailing spaces

a775b83

Update ci-cd.yml

0652d27

webknjaz requested changes Jun 19, 2025

View reviewed changes

Vizonex and others added 8 commits June 19, 2025 16:40

Update Makefile

c0cc056

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

Update .github/workflows/ci-cd.yml

fc3e1be

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

Update .gitignore

d426c89

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

add documentation

dd88a6c

move cython requirement to ci.txt as suggested

fbac986

add cython istr api workaround

69f7c43

if --c-extensions is enabled allow workflow to compile it in pre-exec…

76b63a2

…ution

[pre-commit.ci] auto fixes from pre-commit.com hooks

16701b6

for more information, see https://pre-commit.ci

Vizonex and others added 6 commits June 20, 2025 11:10

satisfy mypy

c04c2f8

statisfy mypy

2279860

[pre-commit.ci] auto fixes from pre-commit.com hooks

78a90cc

for more information, see https://pre-commit.ci

better venv check

f50697a

Merge branch 'Cython-Compatability' of https://github.com/Vizonex/mul…

faa13ac

…tidict into Cython-Compatability

[pre-commit.ci] auto fixes from pre-commit.com hooks

cd5cbfb

for more information, see https://pre-commit.ci

Vizonex mentioned this pull request Jun 20, 2025

CAPI Draft #1183

Draft

Vizonex closed this Jun 20, 2025

Vizonex mentioned this pull request Jun 21, 2025

New MultiDict C-API functions And Added Documentation #1185

Merged

3 tasks

		"""Get multidict headers for compiling multidict with other
		C Extensions or cython code"""

Uh oh!

Conversation

Vizonex commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

webknjaz commented Jun 18, 2025

Uh oh!

asvetlov commented Jun 18, 2025 • edited by webknjaz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vizonex commented Jun 19, 2025

Uh oh!

webknjaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vizonex commented Jun 20, 2025

Uh oh!

Vizonex commented Jun 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Vizonex commented Jun 17, 2025 •

edited

Loading

asvetlov commented Jun 18, 2025 •

edited by webknjaz

Loading