-
Notifications
You must be signed in to change notification settings - Fork 8
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
Python POC #282
Python POC #282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sticking my foot in the door as I really want to review this - feel free to pester me if I haven't started in a week :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Praise: I have only viewed 15 files, but I like what I have read so far.
TestModels/Aggregate/runtimes/python/test/extern/test_dafny_wrapper.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I am skipping over the rest of the test Models for now.
TestModels/SharedMakefile.mk
Outdated
# Hacky workaround until Dafny supports per-language extern names. | ||
# Replaces `.`s with `_`s in strings like `{:extern ".*"}`. | ||
# This is flawed logic and should be removed, but is a reasonable band-aid for now. | ||
# TODO: Once Dafny supports per-language extern names, remove and replace with Pythonic extern names. | ||
# This is tracked in https://github.com/dafny-lang/dafny/issues/4322. | ||
# This may require new Smithy-Dafny logic to generate Pythonic extern names. | ||
_python_underscore_extern_names: | ||
find src -regex ".*\.dfy" -type f -exec sed -i $(SED_PARAMETER) '/.*{:extern \".*\".*/s/\./_/g' {} \; | ||
find Model -regex ".*\.dfy" -type f -exec sed -i $(SED_PARAMETER) '/.*{:extern \".*\.*"/s/\./_/g' {} \; | ||
find test -regex ".*\.dfy" -type f -exec sed -i $(SED_PARAMETER) '/.*{:extern \".*\".*/s/\./_/g' {} \; | ||
|
||
_python_underscore_dependency_extern_names: | ||
$(MAKE) -C $(STANDARD_LIBRARY_PATH) _python_underscore_extern_names | ||
$(patsubst %, $(MAKE) -C $(PROJECT_ROOT)/% _python_underscore_extern_names;, $(LIBRARIES)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Praise/Note:
Nice way to "Make it Work".
This is very close to solution written by an ARG Intern from 2019.
codegen/smithy-dafny-codegen-cli/src/main/java/software/amazon/polymorph/CodegenCli.java
Outdated
Show resolved
Hide resolved
Note: | ||
Python's `.encode('utf-8')` does not handle surrogates. | ||
These methods are expected to handle surrogates (e.g. "\uD808\uDC00"). | ||
To work around this, we encode Dafny strings into UTF-16-LE (little endian) | ||
and decode them before encoding into UTF-8 (`_strict_tostring`). | ||
To decode, we reverse the encode step. (`_reverse_strict_tostring`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Tell me about the performance characteristics of this implementation.
It appears that we parse every character three times.
For Encoding:
- Encode to UTF-16-LE
- Decode to ?
- Encode to UTF-8
For Decode:
- Decode from UTF-8
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are parsing each character 3 times on decode as well (reverse of encode).
(Obviously I'm not happy about this, but I struggled to handle surrogate characters, and this seemed to be the most commonly suggested pattern to handle those...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to block development on this.
But we should not ship a product with this deficiency.
I do not know how often we convert strings,
but I imagine it's often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a good time to at least discuss the eventual path to using --unicode-char:true
in Dafny. A big part of supporting that mode is avoiding exactly this annoying edge case.
...odels/dafny-dependencies/StandardLibrary/runtimes/python/src/standard_library/extern/UTF8.py
Outdated
Show resolved
Hide resolved
...odels/dafny-dependencies/StandardLibrary/runtimes/python/src/standard_library/extern/UTF8.py
Outdated
Show resolved
Hide resolved
...odels/dafny-dependencies/StandardLibrary/runtimes/python/src/standard_library/extern/UTF8.py
Outdated
Show resolved
Hide resolved
…afny into lucmcdon/python-poc
It looks like the Python GHA uses Dafny 4.3 but the Dafny Verification uses 4.1. |
What changes did you have to make to |
Changes are here. |
* chore: add TestModel for SimpleEnumV2 in Python * chore: enable SimpleEnumV2 Python in CI
Python codegen for Smithy-Dafny.
This generates conversion code between Dafny-generated Python code and
The generated code works for all TestModels, the MPL, and MPL TestVectors.
MPL PR: aws/aws-cryptographic-material-providers-library#171
Sub-PRs
These smaller PRs focus on parts of this PR. These are not functional on their own. These are solely intended for reviewer readability.
namespacing soon.) https://github.com/lucasmcdonald3/smithy-dafny/pull/9
customize
directive. This writes new files, and generally does not need to override any Smithy-Python generated code. https://github.com/lucasmcdonald3/smithy-dafny/pull/10.Pending work
TODO-Python-PYTHONPATH
._
to.
(this will fix Java and Net AWS SDK GHA failures)Build instructions
Set up project:
Build a TestModel:
GHA Failures
Misc
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.