-
Notifications
You must be signed in to change notification settings - Fork 25
feat!: typed PRA#run #329
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
base: main
Are you sure you want to change the base?
feat!: typed PRA#run #329
Conversation
…to config-args
ntalluri
left a comment
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 is my first pass of this PR.
Could you add an example demonstrating how these files and components fit together for an example algorithm and its configuration file, and show how the new files are used for validation and execution?
Also, for someone looking to integrate a new algorithm into SPRAS, what details or requirements should they be aware of? I’m assuming the new key piece would involve the Pydantic models.
Co-authored-by: Neha Talluri <[email protected]>
I'm confused by this question: we do this with the present algorithms. |
What I mean by this is writing down in a paragraph what is happening with an example in this PR. Essentially, just spelling out what’s happening step-by-step for an example so it’s clear how everything works together. |
|
We have example usage in the PR description 👍 As for implementation details, the important part is the schema: Algorithm files (e.g. |
agitter
left a comment
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.
I can follow the overall design goals. My big picture takeaway is that I can see why we need these changes, but the typing adds indirection and hurts code readability. I don't have a solution for that.
I haven't looked at every pathway reconstruction algorithm and test case update yet, so I'll need to take at least one more pass. I wanted to leave some initial comments.
algorithms.py has some sophisticated Python. I am fairly sure I understand it when reviewing it today. I'm not sure about my ability to troubleshoot things if/when they break.
Co-authored-by: Anthony Gitter <[email protected]>
Co-authored-by: Anthony Gitter <[email protected]>
…to config-args
agitter
left a comment
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.
My only new comments are small.
In the spirit of helping new contributors who encounter this code base in the future, I'm wondering where to capture some of the information about the overall SPRAS design, especially the part that is changing here. Some of the useful information in the original message of this PR, e.g.
Arguments are now specified as a pydantic BaseModel with attached documentation:
provides a guide to how SPRAS works and where to find things. Is there a place to retain that knowledge in the repo? Scrolling through individual files to reconstruct it is going to get harder and hard.
The best place to document that would be the contributing guide: We could increase the sophistication of our AllPairs wrapping example to take in an optional argument, but I'm not sure what that would be. |
Note
This also contains changes that close #297 to resolve a dependency diamond. I was debating about splitting that to another PR, but that 'containers' PR would depend on #292 (giving this 2 degrees of dependency), and the types in PRA#run actually make that change easier to follow and motivate.
containerskey #387This also borrows (not a direct dependency) from #286. #286 should be merged after #329 is merged, as there's a good chance that we can just auto-generate documentation from the documentation here instead.
Closes #321, closes #296, and closes #297.
Arguments are now specified as a pydantic BaseModel with attached documentation:
When constructing a
PRM, this is passed in as a generic:For algorithms that don't specify parameters, the
Emptytype is preferred instead (whose signature is the emptyBaseModelwith an attachedmodel_configthat doesn't allow any other parameters.This also changes the signature of
PRA#run(reflecting the PR title) to be:This has the disadvantage that
inputsno longer has code completion, but this was probably something to not hide from the developer-user anyway, as we were passing inputs in via kwargs. See:All of this gives us:
For reviewers
algorithms.pyis the only "dense code" in this PR. That file extends our previousevalsystem and includes some hard-to-follow workflow code that enables type checking for our config file.Otherwise, most of this is just refactoring all of the
PRA#runcalls to meet the new signature, specifying the new parameter models, and adding more parameter documentation.