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

158 add ability for plugin pipeline external persistent processes #167

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

Conversation

cfirth-nasa
Copy link
Contributor

Services framework has been implemented and unit tests have been updated to cover modifications. Currently, no example services are provided. Deferring to reviewers on whether we ought to wait for an example to be implemented (and tested) before adding to the main branch. This Services implementation is expected to be fully backwards compatible with previous OnAIR implementations.

@cfirth-nasa cfirth-nasa added the enhancement New feature or request label Nov 14, 2024
@cfirth-nasa cfirth-nasa linked an issue Nov 14, 2024 that may be closed by this pull request
instance2 = Singleton()

# Assert
assert instance1 is instance2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this test perfectly describes "singleton" functionally.

@the-other-james
Copy link
Contributor

Confirmed that tests pass and coverage is 100% on my setup. Code looks good to me (but I wasn't checking for docstrings...).

I created #176 to capture that we still need an example and docs for how to create and use a service.

Great work!

@the-other-james
Copy link
Contributor

@asgibson and I were discussing how plugins access services and want to avoid the user having to explicitly "instantiate" the services singleton.
Would it make sense to update the AI_plugin init function (https://github.com/nasa/OnAIR/blob/main/onair/src/ai_components/ai_plugin_abstract/ai_plugin.py#L20) with the services instantiationg? And then the plugin can just access via something like 'self.services'? Similar to how all plugins have access to headers, the list of telemetry point names.

@cfirthae
Copy link

cfirthae commented Dec 9, 2024

Large update... ServiceManager is now instantiated in the AIPlugin superclass, meaning Plugins can access it by self.service_manager.

This change necessitated a bunch of fixes in the CSV Output example plugin's unit tests since the way those are written is out of date. Preliminary changes to that set of tests were made so it would pass, but they will need a full overhaul eventually - I'll make a new issue.

Merged commits to take advantage of more recent updates to the Pylint pipeline.

We should be good to go!

Copy link
Contributor

@asgibson asgibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a clean rebase with main.

onair/services/service_manager.py Outdated Show resolved Hide resolved
@asgibson asgibson force-pushed the 158-add-ability-for-plugin-pipeline-external-persistent-processes branch from 426d8ba to 67dea87 Compare December 10, 2024 14:49
cfirth-nasa and others added 12 commits December 18, 2024 14:53
    - Add new initialized object in ExecutionEngine, the ServiceManager, which behaves as a singleton
        - When specified in the config (example & tutorial to come in a later commit), a Service is made an attribute of the ServiceManager
        - The ServiceManager can be imported in any plugin. As a singleton, when called to initialize, the original ServiceManager instance (along with instantiated Services) will be returned

    - Services are defined in the config and imported automatically when ServiceManager is first generated
…_services_dict FileNotFoundError message to align with parse_plugins_dict.
- ServiceManager is instantiated in the AIPlugin and set to the attr Plugin.service_manager
    - Plugins no longer need to import it individually and can access by attr instead
- Add corresponding unit test to AI plugin abstract class
@asgibson asgibson force-pushed the 158-add-ability-for-plugin-pipeline-external-persistent-processes branch from 80891f2 to 6a3ff91 Compare December 18, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability for plugin-pipeline external, persistent processes
6 participants