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

Small documentation improvements & quickstart guide #399

Merged
merged 7 commits into from
Feb 1, 2023

Conversation

steve-s
Copy link
Contributor

@steve-s steve-s commented Jan 31, 2023

No description provided.

HPy unit tests
~~~~~~~~~~~~~~

HPy usually has tests for each API function. This means that there is lots of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this paragraph is just shuffled at the end of this file


- **Debugging**: it provides an improved debugging experience. Debug mode can be turned
on at runtime without the need to recompile the extension or the Python running it.
HPy design is more suitable for automated checks.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I shuffled this at the top, shortened the points, and added think "stable ABI" on steroids

- use tagged pointers to reduce memory footprint

Where to go next:
-----------------
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could pitch some parts of the documentation depending on what you're looking for: either code samples or some more explanations/details

embedding
protocols
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two were not a real documentation, but design notes. I don't think it fits in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created an issue for the protocols: #400, and added a comment to the existing str builder issue: #214

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this comment only after my review. Maybe we could move the string stuff to the wiki?

docs/index.rst Outdated Show resolved Hide resolved

There are several advantages to write your C extension in HPy:
- use a tracing garbage collection instead of reference counting
- remove the global interpreter lock (GIL) to take full advantage of multicore architectures
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true? Does using HPy make it any easier to be sure code does not have threading problems?

Copy link
Contributor Author

@steve-s steve-s Feb 1, 2023

Choose a reason for hiding this comment

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

Yes, good point. This bullet point was here before, I just shuffled it and added "... to take full advantage of multicore architectures".

HPy make it any easier to be sure code does not have threading problems

Depends on the point of view. My line of thinking is that it would make it easier to do the internal changes in CPython that are needed to remove the GIL. Still extensions would have to make sure that they operate correctly without GIL.

I think it still falls into the category of "what is wrong with current C API", although better HPy would not solve the problem as a whole, only a part of it. Explaining all this in detail would make this bullet point too long/complex. I like that "removing GIL" is something that many would clearly understand brings benefits and there is bit of a hype around it now :-) I think this simplification could be fine for such a pitch as the beginning of the documentation?

@@ -1,443 +0,0 @@
bytes/str building API
=======================
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this into a "design document" section?

Copy link
Contributor Author

@steve-s steve-s Feb 1, 2023

Choose a reason for hiding this comment

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

I've linked it to the relevant GitHub issue. I think that having design documents in the "main" user documentation is not good, because:

  • it can be misleading and create false expectations
  • we should primarily use GitHub issues (or something else, but a single location -- I haven't read it in detail, but I suspect that str-builder-api.rst is not up-to-date, anyway)

@@ -0,0 +1,52 @@
HPy Quickstart
=============
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this duplicate much of the simple example

Copy link
Contributor Author

@steve-s steve-s Feb 1, 2023

Choose a reason for hiding this comment

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

Yes, but I think the two can serve slightly different purposes: quickstart -- when I am impatient, I want hands on experience right now, I don't mind following instructions blindly for now, simple example -- when I want some additional context and explanations.

I could not resist adding few explanatory comments to quickstart, but I tried to keep them very short.

Copy link
Contributor

@mattip mattip left a comment

Choose a reason for hiding this comment

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

Nice improvements. I suggested a few small changes. I would be fine merging this as-is and considering the quickstart vs. the simple-example later.

I think we should keep the string builder document somewhere, at least until we have a string builder API.

steve-s and others added 2 commits February 1, 2023 11:59
Co-authored-by: Matti Picus <matti.picus@gmail.com>
@steve-s
Copy link
Contributor Author

steve-s commented Feb 1, 2023

Thanks for review @mattip! If the CI jobs pass (modulo cppcheck), I'll go ahead with merge and we can discuss these later (maybe on a dev call):

  • do we want quickstart, how should it look like
  • moving the string builder design document elsewhere

@steve-s steve-s merged commit e832637 into hpyproject:master Feb 1, 2023
@mattip
Copy link
Contributor

mattip commented Feb 1, 2023

Cool, thanks

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.

None yet

2 participants