-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor services entry point to use named arguments #135
base: main
Are you sure you want to change the base?
Conversation
entry.py
Thank you so much @elias-ba ! I look forward to reviewing this tomorrow |
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.
Thanks @elias-ba! This seems to work great. One request please, then I'll ask Hanna to test (this works great with the assistant though)
One question: how do we use entry_test.py
?
platform/src/bridge.ts
Outdated
const proc = spawn( | ||
"poetry", | ||
[ | ||
"run", | ||
"python", | ||
"services/entry.py", | ||
"--service", |
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.
Can we make script name required and position? I think the absolute minimum here is python services/entry.py <service>
, and everything else is probably technically optional (even if we usually use it)
Thanks for the review, @josephjclark! 🙌 I’ve implemented the changes you requested, so everything should be good to go now. Let me know if there’s anything else you’d like adjusted. You can run the pytest services/entry_test.py I have updated the Let me know if you hit any snags or need help with it! Appreciate your feedback, as always! ✌️ |
Thank you @elias-ba 🙏 @hanna-paasivirta when you're next online: Remember that change you made to the arg parser in b383b0d ? Could you repeat your test to expose that? It'll fail on main now but should work against this branch. If you have trouble please let me know ASAP. Thanks! |
Short Description
Refactor Python service entry point to use named arguments instead of positional arguments, making it easier to use, more maintainable, and less error-prone.
Fixes #132
Implementation Details
The Python service entry point (
entry.py
) is a critical part of our system, used both at runtime by the API and during local development. Previously, it relied on positional arguments for service configuration, which caused confusion and made the script harder to maintain.Key Changes
Switch to Named Arguments
The script now uses named arguments via
argparse
, making it more explicit and self-explanatory:<service>
: Required; specifies the service to run.--input
(-i
): Optional; path to the input JSON file.--output
(-o
): Optional; path to the output JSON file (auto-generated if not provided).--port
(-p
): Optional; sets the Apollo server port.Improved Error Handling
Integration
bridge.ts
has been updated to use the new named argument format when spawning Python processes.These changes enhance the usability and maintainability of the entry point while maintaining backward compatibility for existing API.
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy