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

Foundation work for one-step debugger in Workflows #761

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

Conversation

PawelPeczek-Roboflow
Copy link
Collaborator

@PawelPeczek-Roboflow PawelPeczek-Roboflow commented Oct 30, 2024

Description

In this PR we are introducing the following capabilities into EE:

  • decoupling data serialisation and deserialisation from core of EE in favour of kind-based extensions
  • ability to define batch-oriented inputs for any kind and any input dimensionality

Which effectively would allow us to enable debugger experience for Workflows ecosystem

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

  • bunch of new tests in CI for all changes introduced
  • old CI still 🟢

Any specific deployment considerations

For example, documentation changes, usability, usage/costs, secrets, etc.

Docs

  • Docs updated? What were the changes:

from typing import Literal, Union
from pydantic import Field
from inference.core.workflows.prototypes.block import (
WorkflowBlockManifest,
)
from inference.core.workflows.execution_engine.entities.types import (
StepOutputImageSelector,
WorkflowImageSelector,
BatchSelector,
Copy link
Contributor

Choose a reason for hiding this comment

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

We are kind of reintroducing concept of batch which we tried to kind of get rid of / make implicit.

I think I get why this is correct, but have gut feeling about it being hard to understand. I think its correct in that it's more explicit (some selectors can point to batch ordered values vs some that are never, i.e. scalars in second case)

Where I think it makes things harder to understand is that for very common use case of building a block that handles image data, user has to be explicitly aware of Batch concept. Maybe thats OK / better.

Only alternative I can think of at the moment is having e.g. ImageSelector which would essentially be BatchSelector(kind=[IMAGE_KIND]) as a type. That seems a bit more accessible in terms of discovering / learning about and then maybe offers a path to learn about how Batch works (i.e. to explain we can be like "you know how you use ImageSelector...well its really Batch Selector with kind image because e.g consider the dynamic crop block output"). But I don't know maybe its just more confusing to have another type that only fixes the kind of Batch Selector?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's have a call today - I do not really know what would be better - still thinking about it

My main concern is between ScalarSelector(...) and BatchSelector(...) as

this piece of manifest:

field_a: BatchSelector(...)
field_b: ScalarSelector(...)

at the level of run method would have two variants:

  • when step accepts batches - def run(self, field_a: Batch[X], field_b: Y):

  • when step does not accept batches - def run(self, field_a: X, field_b: Y):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I had a thinking session about the problem

  • if not batches we would only have a need to have a DataSelector(...) type annotation - and that would be our universal way of saying - "this field refers to either other step output or workflow input and expect specific data" - that would be awesome, as then you always create run(...) method as if that were any python function
  • ... so basically what we need to overcome is the way of saying - "dear Execution Engine, as a performant block, I am able to apply the operation on multiple instances of data at a time - please provide me batched input" in a way that block always know which input parameters will be batches, which not.
    The solution for that problem could be:
class Manifest(WorkflowBlockManifest):
    # instead
    @classmethod
    def accepts_batch_input(cls) -> bool:
        return True

    # use this
    @classmethod
    def get_batch_oriented_inputs() -> Optional[List[str]]:
        return ["image", "predictions"]

which would virtually inform EE when batch-oriented inputs are needed, but will not pollute 95% of the blocks with distinguishing between BatchSelector(...) or ScalarSelector(...)

  • as a result, EE would have the exact same information as now, but blocks would not need differentiate

unfortunately there are also disadvantages:

  • on UI end, we loose the ability of suggesting batch-oriented data vs scalars as this information is not embedded at the level of fields - we need to evaluate how important that would be - I can image that not particularly important for blocks that do not accept batched input, but for the other group - I still did not discover whole spectrum of side-effects - seems like EE could dynamically broadcast non-batch oriented inputs to align with blocks expectations and the only problematic situation would be when dimensions of input batches do not match - which is already handled by dimensionality guards in EE
  • I am not sure how to introduce the change such that we do not break the ecosystem / cause inconsistencies - I have a feeling that this is the type of change we should apply in execution engine v2

@@ -108,6 +108,65 @@ REGISTERED_INITIALIZERS = {
}
```

## Serializers and deserializers for *Kinds*
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a really powerful and extensible pattern. Wondering if it also opens the door for e.g. kind conversion/casting via serialize as one kind and deserialize as other

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes as an opportunity to hook up this extension
no if you ask if this is possible without any changes to the PR
and probably no - if the question is broader - the polymorphism in kinds type system - that would need to be approached from different angle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants