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

feat/finishing typeddict inputs #95

Merged
merged 7 commits into from
Sep 30, 2024

Conversation

blast-hardcheese
Copy link
Contributor

@blast-hardcheese blast-hardcheese commented Sep 28, 2024

Why

We got pretty close to having TypedDicts for river-python inputs before, but had to roll back due to a protocol mismatch.

Trying again, and also adding some tests to confirm that at the very least the Pydantic models can decode was was encoded by the TypedDict encoders. It's not a perfect science, but it should be good enough to start building more confidence as we make additional progress.

The reason for "janky" tests

There's a bit of a chicken-and-egg situation when trying to test code generation at runtime.

We have three options:

  • write pytest handlers where each invocation runs the codegen with a temp target (like the shell script does here), writes a static file for each text into that directory, then executes a new python into that directory. The challenge with this is that it would suck to write or maintain.
  • write pytest handlers which runs the codegen with unique module name targets (like gen1, gen2, gen3, one for each codegen run necessary) and carefully juggle the imports to make sure we don't try to import something that's not there yet. This might be the best option, but I'm not convinced about the ergonomics at the moment. It might be OK though, with highly targeted .gitignore's.
  • maintain a bespoke test runner, optimize for writing and maintaining these tests, and just acknowledge that we are doing something obscure and difficult.

I definitely wrote the tests here in a way that would give some coverage and also provide confidence, while intentionally deferring the above decision so we can keep making progress. in the meantime.

What changed

  • Added some janky tests for comparing the encoding of both models
  • Fixed many bugs in the TypedDict codegen and encoders

Test plan

$ bash scripts/parity.sh
Using /tmp/river-codegen-parity.bAZ
Starting...
Verified

@blast-hardcheese blast-hardcheese requested a review from a team as a code owner September 28, 2024 04:24
@blast-hardcheese blast-hardcheese requested review from ryantm and removed request for a team September 28, 2024 04:24
@blast-hardcheese blast-hardcheese force-pushed the dstewart/feat/finishing-typeddict-inputs branch 3 times, most recently from 58885f2 to bfb710d Compare September 28, 2024 05:15
@blast-hardcheese
Copy link
Contributor Author

@ryantm I edited the description to explain the odd testing story here

replit_river/codegen/client.py Show resolved Hide resolved
scripts/parity/check_parity.py Show resolved Hide resolved
@blast-hardcheese blast-hardcheese force-pushed the dstewart/feat/finishing-typeddict-inputs branch from c319d0b to 9fefb77 Compare September 30, 2024 18:52
@blast-hardcheese blast-hardcheese merged commit ca9d552 into main Sep 30, 2024
3 checks passed
@blast-hardcheese blast-hardcheese deleted the dstewart/feat/finishing-typeddict-inputs branch September 30, 2024 18:54
@blast-hardcheese blast-hardcheese added the bug Something isn't working label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants