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

Refactor services entry point to use named arguments #135

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

elias-ba
Copy link
Collaborator

@elias-ba elias-ba commented Dec 12, 2024

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

  1. Switch to Named Arguments
    The script now uses named arguments via argparse, making it more explicit and self-explanatory:

    python services/entry.py <service> --input input.json --output output.json --port 5000
    • <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.
  2. Improved Error Handling

    • Clear error messages are provided for common issues like invalid JSON input or missing files.
    • Type hints have been added for better clarity and maintainability.
  3. 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!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

@elias-ba elias-ba changed the title Named arguments for entry.py Refactor services entry point to use named arguments Dec 12, 2024
@josephjclark
Copy link
Collaborator

Thank you so much @elias-ba ! I look forward to reviewing this tomorrow

Copy link
Collaborator

@josephjclark josephjclark left a 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?

const proc = spawn(
"poetry",
[
"run",
"python",
"services/entry.py",
"--service",
Copy link
Collaborator

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)

@elias-ba
Copy link
Collaborator Author

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 services/entry_test.py using pytest like this:

pytest services/entry_test.py

I have updated the pyproject.tomlfile at the root of the project to include pytest. It's a cool little package for writing and running unit tests in Python. So you may probably need to install it by running poetry install before:

Let me know if you hit any snags or need help with it! Appreciate your feedback, as always! ✌️

@josephjclark
Copy link
Collaborator

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!

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.

Fix arguments in entry.py
2 participants