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

separate the sources into dedicated directory, fix doc generation #47

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

tomato42
Copy link
Collaborator

@tomato42 tomato42 commented Jul 22, 2024

for the documentation generation to work, the code needs to work as a package and be importable, and the package needs to have a valid name (kyber-py won't work), so put all of the sources into the src/kyber directory. Upside: clear separation between shipping code and tests. Also, because everything is in single module, it won't pollute python namespace.

also change it to use relative imports so that any future refactoring is easier (and they're recommended anyway)

@tomato42 tomato42 requested a review from GiacomoPope July 22, 2024 18:46
@tomato42 tomato42 marked this pull request as draft July 22, 2024 18:47
@tomato42 tomato42 marked this pull request as ready for review July 22, 2024 18:49
@GiacomoPope
Copy link
Owner

GiacomoPope commented Jul 22, 2024

Thanks for doing this work. The only thing I'm not sure about is calling the package "kyber" but I don't have a great idea about what to do. It's annoying to have to think about the future here as Kyber will be an antiquated name at some point. Maybe mlkem_py, ml_kem_py, py_mlkem or even kyber_py... can't decide and I'm not super happy with any of them

@GiacomoPope
Copy link
Owner

Also I think I prefer the absolute rather than relative paths to functions but if relative is the standard then I guess I can accept this.

@tomato42
Copy link
Collaborator Author

Thanks for doing this work. The only thing I'm not sure about is calling the package "kyber" but I don't have a great idea about what to do. It's annoying to have to think about the future here as Kyber will be an antiquated name at some point. Maybe mlkem_py, ml_kem_py, py_mlkem or even kyber_py... can't decide and I'm not super happy with any of them

I still see plenty of code that refers to AES as Rijndael... 🙂 and kyber definitely rolls of the tongue better than ML-KEM, unlike Rijndael vs AES...

The thing is, that the package name in PyPI is kyber-py, so the principle of least surprise is that the top level module name should have at least kyber in it. But if you'd prefer kyber_py I can change to that... with relative imports that will be fairly easy 😉

@GiacomoPope
Copy link
Owner

Yeah ok let's do kyber_py for the package name. I don't think that we should "package" this properly until the final spec is out but it doesn't hurt to pick a name now.

@tomato42
Copy link
Collaborator Author

Well, I'd prefer it as a package for integrating into tlslite-ng...

@GiacomoPope
Copy link
Owner

Sure I appreciate that. Originally I was very against this being an importable package as I wanted to dissuade people from using it for cryptography but let's see what state the code is in after a little more work.

@tomato42 tomato42 force-pushed the fixup-doc-gen branch 2 times, most recently from d551c79 to 628dcad Compare July 23, 2024 09:36
@GiacomoPope
Copy link
Owner

Thanks for changing the name.

Could we also tweak the examples in the README so the imports are correct for users?

>>> from ml_kem import ML_KEM128
>>> ek, dk = ML_KEM128.keygen()
>>> key, ct = ML_KEM128.encaps(ek)
>>> _key = ML_KEM128.decaps(ct, dk)
>>> assert key == _key

to something like:

>>> from kyber_py.ml_kem import ML_KEM128
>>> ek, dk = ML_KEM128.keygen()
>>> key, ct = ML_KEM128.encaps(ek)
>>> _key = ML_KEM128.decaps(ct, dk)
>>> assert key == _key

for the documentation generation to work, the code needs to work
as a package, and the package needs to have a valid name
(kyber-py won't work), so put all of the sources into the src/kyber
directory. Upside: clear separation between shipping code and tests.
Also, because everything is in single module, it won't pollute
python namespace

Signed-off-by: Hubert Kario <[email protected]>

# Conflicts:
#	src/kyber_py/kyber/kyber.py
#	src/kyber_py/ml_kem/ml_kem.py
@tomato42 tomato42 force-pushed the fixup-doc-gen branch 2 times, most recently from 6683069 to 28fcce4 Compare July 23, 2024 09:53
@tomato42
Copy link
Collaborator Author

@GiacomoPope also docs needed fixing, I've added them to CI now so it will be clear if/when they get broken again :)

@GiacomoPope GiacomoPope merged commit c8820db into GiacomoPope:main Jul 23, 2024
6 checks passed
@tomato42 tomato42 deleted the fixup-doc-gen branch July 23, 2024 10:04
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