Skip to content
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

Compile code with Mypyc #165

Closed
wants to merge 2 commits into from
Closed

Compile code with Mypyc #165

wants to merge 2 commits into from

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Jan 5, 2022

Hello again! Let me know what you think 😄

This PR compiles code with Mypyc. To do so, I've switched the build system to hatchling in order to use the hatch-mypyc build hook plugin. Hatchling is the minimal build backend of Hatch (v1 rewrite of CLI being released next week). You can think of it as the same distinction between flit-core & flit or poetry-core & poetry, with hatchling being more similar to flit-core in that only accepted standards are used e.g. project metadata format (PEP 621), dependency specification (PEP 631), etc.

I added binary wheels to the release workflow using the now ubiquitous cibuildwheel. Here is an example of the files produced: https://pypi.org/project/hatch-showcase/#files

The only thing I couldn't preserve was the hack where we change the __module__ of TOMLDecodeError.

On my Windows machine, tox -e benchmark shows a 2-3x performance improvement.

@ofek ofek force-pushed the mypyc branch 5 times, most recently from 455414a to 8f8bef9 Compare January 5, 2022 05:08
description = "A lil' TOML parser"
authors = [
{ name = "Taneli Hukkinen", email = "[email protected]" },
]
license = { file = "LICENSE" }
license = "MIT"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supports https://www.python.org/dev/peps/pep-0639/

File automatically included

- name: Build and check
run: |
rm -rf dist/ && python -m build
twine check --strict dist/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -125,10 +125,10 @@ class Flags:
"""Flags that map to parsed keys/namespaces."""

# Marks an immutable namespace (inline array or inline table).
FROZEN = 0
FROZEN: ClassVar = 0
Copy link
Contributor Author

@ofek ofek Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tomli\_parser.py:279: error: Cannot access instance attribute "EXPLICIT_NEST" through class object
tomli\_parser.py:279: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:279: error: Cannot access instance attribute "FROZEN" through class object
tomli\_parser.py:281: error: Cannot access instance attribute "EXPLICIT_NEST" through class object
tomli\_parser.py:281: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:297: error: Cannot access instance attribute "FROZEN" through class object
tomli\_parser.py:297: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:302: error: Cannot access instance attribute "EXPLICIT_NEST" through class object
tomli\_parser.py:302: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:323: error: Cannot access instance attribute "EXPLICIT_NEST" through class object
tomli\_parser.py:323: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:327: error: Cannot access instance attribute "EXPLICIT_NEST" through class object
tomli\_parser.py:327: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:329: error: Cannot access instance attribute "FROZEN" through class object
tomli\_parser.py:329: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:342: error: Cannot access instance attribute "FROZEN" through class object
tomli\_parser.py:342: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:437: error: Cannot access instance attribute "FROZEN" through class object
tomli\_parser.py:437: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)
tomli\_parser.py:453: error: Cannot access instance attribute "FROZEN" through class object
tomli\_parser.py:453: note: (Hint: Use "x: Final = ..." or "x: ClassVar = ..." to define a class attribute)

Comment on lines -177 to -178
cont = cont[key_stem]
return flag in cont["flags"] or flag in cont["recursive_flags"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError: dict object expected; got set

Comment on lines -320 to -321
relative_path_cont_keys = (header + key[:i] for i in range(1, len(key)))
for cont_key in relative_path_cont_keys:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tomli\_parser.py:320: warning: Treating generator comprehension as list

Comment on lines -8 to -9
# Pretend this exception was created here.
TOMLDecodeError.__module__ = __name__
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tomli\__init__.py:9: error: Only class variables defined as ClassVar can be assigned to

@hukkin
Copy link
Owner

hukkin commented Jan 5, 2022

Hey, wow, thanks for the PR!

On my Windows machine, tox -e benchmark shows a 2-3x performance improvement.

Niiice! This should mean almost the same performance as the C++ and Rust extended pytomlpp and rtoml. With nothing but Python code. Actually pretty amazing!

Let me know what you think

Unfortunately, I think it may be a bad move to merge this into Tomli right now:

  • Tomli has major role in PEP517 based Python bootstrapping right now. Getting that right has involved considering different build-backends, vendoring options, bootstrap processes where PYTHONPATH is manipulated etc. There's many tickets about the struggles we've faced in Tomli's, Flit's and I think even pip's issue tracker. Now that we've got a set-up that seems to satisfy repackagers, I'm hesitant to change anything.
  • I believe this would mean a small bit of extra work for repackagers, adapting their processes, which I'd like to spare them from right now.

If #141 goes as I hope, it could be very interesting to return to this in a few years, when Tomli's role in the bootstrapping processes is replaced by a stdlib parser.

And yeah thanks for hatch-mypyc in general! Some months ago I was looking for a way to mypycify mdformat but there wasn't a way that allowed me to keep pyproject.toml, PEP621 metadata etc. I'll probably try these changes out in that repository soon.

@ofek
Copy link
Contributor Author

ofek commented Jan 6, 2022

@hukkin Fixed 😄 I added https://ofek.dev/hatch/latest/config/build/#conditional-execution and now the build hook is disabled by default

@hukkin
Copy link
Owner

hukkin commented Jan 6, 2022

Ah sorry for not being clear enough. I think it may not be a good idea to switch build backend right now.

@ofek
Copy link
Contributor Author

ofek commented Jan 6, 2022

Oh I see. Any concern in particular or just not the right time?

@hukkin
Copy link
Owner

hukkin commented Jan 6, 2022

My concern is PEP517 bootstrapping issues of distro packagers, where Tomli and its build backend play a big part. The lack of a standard library TOML parser has been a pain for distro packagers, and my understanding is now that flit started vendoring Tomli, they seem somewhat content with the current state. I wouldn't want to disrupt them right now.

Tomli is very widely repackaged now, so changing the build backend means a small bit of extra busywork for a fairly large amount of people.

As soon as a TOML parser moves into the standard library the situation should be very different.

@ofek
Copy link
Contributor Author

ofek commented Jan 6, 2022

Okay, feel free to close this out!

Just so I understand though, can you give an example of what a distro packager would need to do if this was merged? I want hatchling to be as easy to use or package as possible. Is it just a matter of adding the ability to e.g. apt install python-hatchling?

@hukkin
Copy link
Owner

hukkin commented Jan 6, 2022

I'm not a distro packager so I'm not the right person to ask really. But for each package the distros tend to have a file that looks something like this https://src.fedoraproject.org/rpms/python-tomli/blob/rawhide/f/python-tomli.spec where they describe build requirements, how to build, how to install etc. These files may need to adapt if build backend changes. Also for packages like build, flit_core, tomli, also hatchling are special cased a lot because they need to be built before the "regular" build tools exist.

Is it just a matter of adding the ability to e.g. apt install python-hatchling?

Yes this is another thing that needs to happen.


Don't quote me on any of this. I am NOT a distro packager 😄

@ofek
Copy link
Contributor Author

ofek commented Jan 6, 2022

Thanks! I'll work on that next.

@ofek ofek closed this Jan 6, 2022
@ofek ofek deleted the mypyc branch January 6, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants