Orchestrator - Load a JSON of copy phrases to use during execution #352
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
Add TaskMode, fix paradigm definitions, add loading of phrase.json, update task/README.md with orchestrator instructions and usage with phrases.json.
TODO: add tests for multi phrases
tab-cmd
commented
Oct 1, 2024
| @@ -410,31 +410,41 @@ | |||
| "type": "float" | |||
| }, | |||
| "font": { | |||
| "value": "Consolas", | |||
There was a problem hiding this comment.
I removed this since it is not available without installing a new font.
lawhead
approved these changes
Oct 2, 2024
| for sym, sti in self.sti.items(): | ||
| sti.pos = animation_paths[sym][frame] | ||
| sti.draw() | ||
| self.draw_static() |
There was a problem hiding this comment.
The initial reason the draw_static was after the stimuli was to meet performance requirements. Generally the more objects that are drawn the longer it takes. It's possible that with some of the refactoring I did a while back the performance is within spec if static is drawn first, but this would be worth measuring at some point.
bcipy/task/__init__.py
Outdated
|
|
||
| # Makes the following classes available to the task registry | ||
| from .registry import TaskRegistry | ||
|
|
||
| __all__ = [ | ||
| 'Task', | ||
| 'TaskRegistry', | ||
| 'TaskData' | ||
| 'TaskData', | ||
| "TaskMode" |
There was a problem hiding this comment.
Looks like TaskMode is the only one here in double quotes.
| for task in tasks: | ||
| self.add_task(task) | ||
| self.ready_to_execute = True |
There was a problem hiding this comment.
I don't think this self.ready_to_execute is needed since this method is doing a self send to a method that already sets this.
There was a problem hiding this comment.
This is to prevent execution without any Tasks defined; before execution has occurred, more tasks can be added via a list of Tasks or a single Task, and then all tasks will be reset. This allows a caller to continue using the orchestrator after completing some protocol. I will rethink this as it would technically constitute a new/modified protocol.
| } | ||
| """ | ||
| # load copy phrases from json file or take the task text if no file is provided | ||
| if self.parameters.get('copy_phrases_location') != "": |
There was a problem hiding this comment.
Empty strings are treated as False so this can be shortened to:
if self.parameters.get('copy_phrases_location'):
tab-cmd
added a commit
that referenced
this pull request
Oct 23, 2024
* gets `main.py` functionality working in the orchestrator * Fixes task initialization process Save folder and parameters are now saved and loaded per task as they are executed rather than once per session. * Moves parameter loading to task execution * Adds functions for parsing visit actions * Adds some test to action parsing * Adds tests for action serialization * Adds parameter validation function with tests * Updates changelog.md * lints actions.py and test_actions.py * Adds constant for action separator * removes old file that has been moved * renames action parsing module and updates test imports * simplifies `serialize_actions` function * Fixes typing and formatting issues * removes `parse_actions.py` * renames `parse_actions` to `parse_sequence` * replaces magic '->' string with constant * removes unnecessary error check * Adds basic structure for orchestrator actions * refactors config function naming and adds support for actions in parsing functions * renames `test_actions` to `test_config` * Adds additional tests for action parsing * Adds action serialization tests * Add more actions * Refactors type unions to be `Optional`s * Adds parameter loading to the orchestrator * Adds initialize and cleanup methods to `Task` * makes `name` a property in `OfflineAnalysisAction` * Implements `setup` method in base `Task` class * Adds missing parent `execute` call * changes `initialize` call to `setup` in orchestrator * Refactors `name` to be a property in base `Task` class * Refactors `config.py` to only work with tasks * adds demo file for orchestrator execution * Fixes data_save initialization in orchestrator * Creates default_user_id constant * Fixes incorrect user initialization * Refactors orchestrator save structure * Implements `save` method * Adds `task_registry_dict` to simplify fetching classes for initialization This is currently redundant with the `TaskType` approach and ideally would be refactored to have only one source of string reference * Adds comment about `task_registry_dict` * Moves `task_registry_dict` to `orchestrator.config` * refactors `name` property of tasks * Adds task registry class * Refactor `super` calls in orchestrator actions * adds type hint for task parameter * calls `construct_command` in constructor * adds function to run experiment data collection without running from the command line * adds `ExperimentFieldCollectionAction` * refactors orchestrator demo file to use `ExperimentFieldCollectionAction` * lints `task_registry.py` * imports `Task` for correct typing in config * Updates `task_registry.py` to import all tasks * moves `TASK_SEPERATOR` to `config.py` * Adds docstring to orchestrator * Renames `TestConfig` * removes property decorator in task class * removes implementation of abstract class * updates changelog * removes super calls to abstract method `execute` * Filters out abstract subclasses from registry generation * removes registry dict empty initialization * Fixes typing issues * Removes task import from the registry * Refactors config to support TaskRegistry * Refactors task names * Refactors `TaskRegistry` to recursively collect subclasses This makes it compatible with the new `BaseCalibrationTask` as it will be able to collect grandchild classes of `Task` (which is all calibration tasks now) * Makes `BaseCalibrationTask` abstract * removes name and label from `timing_verification.py` * imports tasks in their module's `__init__.py` This makes all tasks available to the task registry when it is initialized. * Enforces `name` attribute in base task class * adds calibration task check to the registry. TaskType now no longer exists * fixes incorrect method name in TaskRegistry * updates bci_main_tests to call main methods properly * updates method calls in protocol parsing tests * refactors `start_app` to use `start_experiment_field_collection_gui` * changes orchestrator directory name to work on windows * Adds basic selection box for orchestrator protocol * Adds the ability to add and remove tasks to the protocol * renames `sequence` to `protocol` in orchestrator config * Allows protocol to be saved to `experiments.json` * adds move up and down controls to tasks in protocol * Refactors scrollable frames to have a title label. Some buttons are still misaligned in the ExperimentRegistry * Moves create experiment button to the bottom of the gui * changes task and action names to be more consistent * adds basic `TaskData` return object * fixes failing test in `test_copy_phrase` * fixes missing return statement for `construct_command` * adds `task_dict` to `TaskData` * updates changelog * adds seperate BCIUI file with stylsheet This will hopefully replace BCIGui later, but I am keeping it seperate for now as to not break existing uis on this branch * update demo experiment layout * adjust stylesheet to remove label backgrounds * improves gui code to allow for editing fields * add basic removeable element * add dynamic list class for easy ui reactivity * Add small button component * removes explicit color from small button * adds basic toggle class * changes toggle componenet to be simple connecting function * makes on off callbacks optional in `make_toggle` * add toggle to optional button * swaps confusing parameter names * loads experiment fields into combobox * adds field buttons * refactors gui to be inherited class This improves state handling, as variables and widgets can be stored on the class itself * add simple `show_alert` method * add show_alert method * adds validation to added fields * adds experiment creation to application * rename `gui_rewrite` to `bciui` * adds app start method * adds vertical center option to bciui * add the beginning of intertask action ui * add progress text * allows for additional args in app running * add functional progress bar * Add next task display and functioning buttons * adds seperate BCIUI file with stylesheet This will hopefully replace BCIGui later, but I am keeping it seperate for now as to not break existing uis on this branch * update demo experiment layout * adjust stylesheet to remove label backgrounds * improves gui code to allow for editing fields * add basic removeable element * add dynamic list class for easy ui reactivity * Add small button component * removes explicit color from small button * adds basic toggle class * changes toggle componenet to be simple connecting function * makes on off callbacks optional in `make_toggle` * add toggle to optional button * swaps confusing parameter names * loads experiment fields into combobox * adds field buttons * refactors gui to be inherited class This improves state handling, as variables and widgets can be stored on the class itself * add simple `show_alert` method * add show_alert method * adds validation to added fields * remove debug prints * adds experiment creation to application * remove unused code * rename `gui_rewrite` to `bciui` * adds app start method * adds vertical center option to bciui * add beginning of intertask action ui * add progress text * allows for additional args in app running * add functional progress bar * Add next task display and functioning buttons * add protocol layout and fix bug in DynamicList * Parameter guardrails and cleanup (#340) * supports adding tasks to the interface * add reordering to tasks in the protocol * allows adding new fields through the ui * adds saving protocol in experiment from gui * Task initialization and cleanup (#343) * Adds documentation to ui methods * rename `intertask_action` to `intertask_gui` * add action that displays intertask GUI * Add proper types and docs to bciui * Deprecates old ExperimentRegistry in favor of new ui code * Fixes incorrect background color when using dark mode * improve stylsheet loading to use existing bcipy constants * update legacy BCInterface to base session-orchestrator * Session Orchestrator Updates [Post-Testing] (#344) * Add logger to each task run * Bcipy client refactor (#346) * Task specific params and Matrix Layout (#349) * Orchestrator - Load a JSON of copy phrases to use during execution (#352) * fix logger assignment * BciPy report action, offline analysis updates, Intertask GUI. (#354) * updates to include IP changes * Update to get static offset from the device --------- Co-authored-by: Carson Reader <carsonthemonkey@gmail.com> Co-authored-by: lawhead <lawhead@ohsu.edu>
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Overview
This PR adds new functionality to load a JSON of copy phrases. I have also fixed several task font bugs and VEPCalibration issues stemming from the PR for separated parameters. This adds an official TaskMode enum to support this feature.
Ticket
https://www.pivotaltracker.com/story/show/188152705 (slight divergence from the discussed approach, using parameters instead of an experiment)
Contributions
Test
Documentation
Changelog