refactor(api): extract RTP definitions and set values during runner load #15217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
…oad (#15217) Addresses AUTH-282 # Overview First part of the two-part changes required to make RTP values be available earlier during analysis. This PR moves extraction of RTP definitions and setting the values of the params to run load stage. This makes sure that the final RTP values are available to the runner before the run is played. # Test Plan Mainly need to make sure that current protocol execution hasn't changed from any client perspective. Unit and integration tests passing should be enough. # Review requests - usual code review and make sure I am not missing anything related to the python protocol execution pipeline # Risk assessment Low. Refactor
4 tasks
Carlos-fernandez
pushed a commit
that referenced
this pull request
Jun 3, 2024
…oad (#15217) Addresses AUTH-282 # Overview First part of the two-part changes required to make RTP values be available earlier during analysis. This PR moves extraction of RTP definitions and setting the values of the params to run load stage. This makes sure that the final RTP values are available to the runner before the run is played. # Test Plan Mainly need to make sure that current protocol execution hasn't changed from any client perspective. Unit and integration tests passing should be enough. # Review requests - usual code review and make sure I am not missing anything related to the python protocol execution pipeline # Risk assessment Low. Refactor
sanni-t
added a commit
that referenced
this pull request
Jun 10, 2024
) Closes AUTH-282 # Overview Follow-up to #15217 This PR changes a *pending analysis'* `AnalysisSummary` to include the run time parameters (with overrides). In order to do this, we will now be parsing the RTPs before analysis response is created. This is a very quick process and shouldn't cause any perceptible delays in the response of neither `POST /protocols` nor `POST /runs/.../analyses` endpoints, as long as recommended python protocol guidelines are followed. # Test Plan - [ ] Upload a protocol with run time parameters, verify that the response contains run time parameter definitions along with the values specified in the request - [ ] Send a `POST` request on `/protocols/{protocolId}/analyses` endpoint with different RTP values, or by specifying the `forceReanalysis` flag, and verify that the newly pending analysis also contains (updated) RTPs - [ ] Verify that pending summaries in `GET` endpoints also contain RTPs - [ ] Verify that summaries of completed analyses don't expose RTPs, while RTPs are available in the full analyses. # Changelog - Main change is the introduction of `AnalysesManager`. The analyses manager is responsible for creating a protocol analyzer per analysis, extracting the run time parameters from the runner and starting the analysis. - To facilitate this extraction of run time parameters, the `ProtocolAnalyzer` has been divided into two functions- one that loads the runner and other that runs the runner. - Updated the router to use `AnalysesManager` for starting analyses instead of directly interacting with `ProtocolAnalyzer`. # Review requests - Does the new architecture of `AnalysesManager` & updated `ProtocolAnalyzer` makes sense. - This PR doesn't expose the run time parameters in analysis summaries of completed analyses because of two reasons- (1) as opposed to the RTPs of a pending analysis, the completed analysis isn't guaranteed to be cached in memory. So we would need to read the completed analysis from database, de-serialize it and then return it. This would be made easier when we start storing RTPs in their own database table, but we're not there yet. (2) I'm not sure if it is needed. A completed analysis already contains the RTPs. Would like to get others' opinions on this. - Usual code review # Risk assessment - Medium. It changes a core part of protocol analysis but the end effect is a small change to the API. As long as tests are passing, it shouldn't cause any problems. - Small risk of analysis behavior change for badly written protocols as mentioned in [this](#15229 (comment)) comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses AUTH-282
Overview
First part of the two-part changes required to make RTP values be available earlier during analysis.
This PR moves extraction of RTP definitions and setting the values of the params to run load stage. This makes sure that the final RTP values are available to the runner before the run is played.
Test Plan
Mainly need to make sure that current protocol execution hasn't changed from any client perspective. Unit and integration tests passing should be enough.
Review requests
Risk assessment
Low. Refactor