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

Make codegen deterministic #162

Merged
merged 5 commits into from
Sep 22, 2023
Merged

Conversation

DXsmiley
Copy link
Contributor

I checked out the repo and, without changes, ran atproto gen all and unexpectedly ended up with a bunch of modified files. It looks like it's mostly just large chunks being moved around inside atproto/xrpc_client/namespaces/sync_ns.py though.

I suspect the culprit is os.walk but I'm not sure. Unfortunately I think it's a system-dependent thing so I don't quite have the ability to test this thoroughly on my machine.

This doesn't impact library users, but I would like to make a few other changes to the codegen without producing a completely unreadable diff.

@MarshalX
Copy link
Owner

What should we do to be sure?

@DXsmiley
Copy link
Contributor Author

If you checkout my branch, run atproto gen all and something changes we'll know there's unfixed problems. If there's no changes then it's hard to say for certain that everything's fixed but it'll give me a lot of confidence.

I think also adding a github action that runs the codegen and checks there's been no changes could also prevent non-determinism being reintroduced in the future. (and should also catch any instances of people making changes and then just forgetting to run the codegen script).

@MarshalX
Copy link
Owner

Could you implement this GitHub actions workflow in this PR please?

@DXsmiley
Copy link
Contributor Author

@MarshalX done. I think it needs your manual approval before it'll actually run though.

@MarshalX
Copy link
Owner

Awesome! Could we run it in matrix on Linux macOS and Windows? To be sure that there is no difference between platforms

@DXsmiley
Copy link
Contributor Author

done! needs another approval though

@DXsmiley
Copy link
Contributor Author

made changes in accordance with https://github.com/snok/install-poetry#running-on-windows to try and get poetry working

@MarshalX
Copy link
Owner

problem of python 3.7. as ez fix (and end of life of python 3.7) let's use python 3.8 here? https://www.mail-archive.com/python-bugs-list@python.org/msg426517.html

@MarshalX MarshalX changed the title Make Codegen Deterministic (I Hope?) Make codegen deterministic Sep 22, 2023
@MarshalX MarshalX merged commit fd70145 into MarshalX:main Sep 22, 2023
7 checks passed
@MarshalX
Copy link
Owner

thank you!

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