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

Combine context.plans and context.plan_functions into a single dictionary #180

Open
DiamondJoseph opened this issue May 12, 2023 · 14 comments
Labels
enhancement New feature or request python Pull requests that update Python code

Comments

@DiamondJoseph
Copy link
Contributor

Making context.plans hold a dataclass e.g.

@dataclass
class PlanWithMetadata:
    plan_function: PlanGenerator
    params: 
    metadata:

prevents having to maintain two dictionaries but also means we can easily extract the serialisable parts of the plan to send.

@DiamondJoseph DiamondJoseph added the enhancement New feature or request label May 12, 2023
@callumforrester
Copy link
Contributor

Also need to integrate the Plan and PlanModel classes

@callumforrester
Copy link
Contributor

I think what's needed is something like

@dataclass
class Plan:
    model: PlanModel
    func: PlanGenerator

@stan-dot stan-dot self-assigned this Mar 8, 2024
@stan-dot
Copy link
Contributor

note: Plan is in core/bluesky_types.py with fields: name, description, model (Pydantic model)
PlanModel is in service/model.py under with fields: name, description, parameter_schema (optional dictionary).
The latter also has a classmethod 'from_plan', which serves as alternative constructor.

Both classes extend BlueepiBaseModel

@stan-dot
Copy link
Contributor

suggested plan of work:

  1. change the PlanModel fields to conform to @DiamondJoseph 's formulation above
  2. change all instances of Plan into the PlanGenerator (?)
  3. Do something about the two other classes in bluesky_types.py: DataEvent and WatchableStatus. Those could be moved into the Model class

@callumforrester
Copy link
Contributor

Need to ensure that there is still a serialisable artefact (i.e. does not hold a function) that can be used as part of the API

@stan-dot
Copy link
Contributor

explore avenue to add a field exempt from serialization to the pydantic model. also a pydantic model not a dataclass.

@stan-dot
Copy link
Contributor

that's tightly connected with #407 , ideally it's 1 PR for both

@callumforrester
Copy link
Contributor

For #407, see #206 (comment)

@stan-dot stan-dot added python Pull requests that update Python code hackathon-ready labels Apr 4, 2024
@stan-dot
Copy link
Contributor

stan-dot commented Apr 4, 2024

discussion output - serialize less, do not except fields, just do composition

@DiamondJoseph
Copy link
Contributor Author

May be able to make use of TypeAdapter instances to get the JSON schema from.

class Plan(BlueapiBaseModel):
    """
    A plan that can be run
    """

    name: str = Field(description="Referenceable name of the plan")
    description: str | None = Field(
        description="Description/docstring of the plan", default=None
    )
    model: type[BaseModel] = Field(
        description="Validation model of the parameters for the plan"
    )

becomes

class Plan(BlueapiBaseModel):
    """
    A plan that can be run
    """

    name: str = Field(description="Referenceable name of the plan")
    description: str | None = Field(
        description="Description/docstring of the plan", default=None
    )
    type_adapter
    generator: PlanGenerator

    @property
    def schema(self) -> str:
        return self.type_adapter.json_schema()

@stan-dot
Copy link
Contributor

stan-dot commented Sep 9, 2024

@DiamondJoseph what if we just convert the plangenerator function to string?

@DiamondJoseph
Copy link
Contributor Author

@DiamondJoseph what if we just convert the plangenerator function to string?

@stan-dot the plan generator was the actual code that was being executed by the RunEngine, but now that it's the opposite side of the _rpc call it's not completely unreasonable. Then

class Plan(BlueapiBaseModel):
    ref: tuple[str, str]  # module and func name? Require module name if func name not unique?
    description: str  # from the docstring
    adapter or schema?

@callumforrester
Copy link
Contributor

So the two dictionaries were originally made so separate the API object, that is passed in and out from external callers, and an internal object with non-serializable data in it to be passed around internally. As @DiamondJoseph says, now we have internal RPC between two processes that makes less sense.

Another option is to try and keep the non-serializable data only in the worker process and never need to pass it around, if that's achievable. We are currently having this discussion because we're thinking about how to get plan information from BlueskyContext to TaskWorker, but I raised #616 to tidy up BlueskyContext anyway, so maybe there's a cleaner way to do all of this?

@stan-dot
Copy link
Contributor

stan-dot commented Sep 9, 2024

BlueskyContext is a god-object because it's too generic a name. TaskWorker will likely have its own context
BlueapiServer will have its own context.

shared state can be held in something like https://github.com/etcd-io/etcd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python Pull requests that update Python code
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants