Bcipy client refactor #346
Merged
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
tab-cmd
force-pushed
the
bcipy_client_refactor
branch
from
70bb087 to
cfa6b46
Compare
lawhead
approved these changes
Sep 16, 2024
| cmd = ( | ||
| f'bcipy ' | ||
| f'-u "{self.user}" -e "{self.experiment}" -p "{self.parameter_location}"' | ||
| ) |
There was a problem hiding this comment.
subprocess.Popen can take either a string or a sequence of args. In this case most of these values are the same, so it might make more sense for the conditional to just add the things that are different.
cmd = ['bcipy', '-u', self.user, '-p', self.parameter_location]
if self.task != BCInterface.default_text:
cmd.extend(['-t', self.task])
else:
cmd.extend(['-e', self.experiment])
bcipy/helpers/system_utils.py
Outdated
| @@ -208,21 +208,21 @@ def configure_logger( | |||
| logfile = os.path.join(save_folder, 'logs', log_name) | |||
|
|
|||
| # configure it | |||
| root_logger = logging.getLogger() | |||
| root_logger.setLevel(log_level) | |||
| custom_logger = logging.getLogger(name=save_folder) | |||
There was a problem hiding this comment.
I think it's good to specify which logger we are configuring, but since you can define more than one handler per logger, consider making the name of the logger different than the save_folder name, which only affects the FileHandler. We may need an additional parameter for that.
custom_logger = logging.getLogger(name=logger_name)
|
Thanks for working on the logging code. 👍 |
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 <[email protected]> Co-authored-by: lawhead <[email protected]>
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.
Overview
Updated bci_main to work with orchestrator instead of old task execution methods. Update load_users and bci-gui to reflect changes. Refactor logging practices were applicable to support saving in task logs etc.
Ticket
https://www.pivotaltracker.com/story/show/187744879
https://www.pivotaltracker.com/story/show/187886760
https://www.pivotaltracker.com/story/show/188151705
Contributions
Test
make test-allbcipy: tested command with fake and real data. Additionally, verified both a single Task and Experiment could be executed correctlyTime Test: both matrix and rsvp were time tested. All are within normal limits.Documentation
Changelog