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

Workflow RDF #310

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

Workflow RDF #310

wants to merge 24 commits into from

Conversation

FynnBe
Copy link
Member

@FynnBe FynnBe commented Nov 4, 2022

needs bioimage-io/spec-bioimage-io#478

Implements resolved nodes for the workflow RDF.

Copy link
Contributor

@oeway oeway left a comment

Choose a reason for hiding this comment

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

Looks good! One suggestion, for running workflow, would be nice to support running it step by step via an python iterator.

For example, for debugging a workflow, the user of the core library can do something like this:

for step in workflow:
print(workflow.context) # print the current available context, e.g. previous outputs, workflow RDF source
print(step.inputs, step.options)
step.execute() # run the step, can also use try except to capture error
print(step.outputs)

What do you think?

@FynnBe
Copy link
Member Author

FynnBe commented Nov 5, 2022

What do you think?

not sure if this really provides value. For debugging one can simply add a breakpoint in the for loop of the run_workflow op... Not sure it's a good idea to basically recommend debugging with print...
If we want this we can of course add this, but I wouldn't conside thist urgent, as we are not sure how to proceed from this draft yet.

@oeway
Copy link
Contributor

oeway commented Nov 5, 2022

Well, think about a user use our package without access to the source code (which is the case for running in triton server).

This is not about debugging with print, you can use pdb the same way. Also, keep in mind that the run_workflow is inside our package. Vscode for example cannot add breakpoint for pip packages, but it can add breakpoint to user's own code. Even if we can add breakpoints for packages, not everyone uses an IDE. We don't want to ask everyone to clone our repo and tap into our source code, just for debugging a workflow.

For triton inference server, we pack the core into a conda package, and it runs inside a docker container, no pdb, no ide, print is the only way to debug. I would say this would be an important feature practically. Especially in the beginning, we want to know what happens when running the workflow.

Having the for loop in users hand can also allow easy UI integration, e.g. showing progress, customize error message for the failed steps. Even more powerful part would be change how we run the workflow steps, for example, I can replace the model execution step and send it to a triton model, or another triton server depending on the current workload.

It's a bit similar to how we train a model. We can of course use callbacks+run_workflow just like the Keras model.fit+callbacks, but often having pytorch-like user training loop provides more flexibility for debugging and usage.

@FynnBe
Copy link
Member Author

FynnBe commented Nov 6, 2022

You raise some good points I didn't think of yesterday. I'll look into implementing the workflow steps with yield... 👍


@staticmethod
def transform_ResolvedImportableSourceFile(node: raw_nodes.ResolvedImportableSourceFile) -> nodes.ImportedSource:
def transform_ResolvedCallableFromSourceFile(node: raw_nodes.ResolvedCallableFromSourceFile) -> nodes.ImportedCallable:
Copy link
Contributor

Choose a reason for hiding this comment

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

[blackfmt] reported by reviewdog 🐶

Suggested change
def transform_ResolvedCallableFromSourceFile(node: raw_nodes.ResolvedCallableFromSourceFile) -> nodes.ImportedCallable:
def transform_ResolvedCallableFromSourceFile(
node: raw_nodes.ResolvedCallableFromSourceFile,
) -> nodes.ImportedCallable:

@FynnBe
Copy link
Member Author

FynnBe commented Feb 9, 2023

most content---workflow implementations and workflow exectuion logic--was moved to https://github.com/bioimage-io/workflows-bioimage-io-python
the resolved nodes remain.

@FynnBe
Copy link
Member Author

FynnBe commented Feb 9, 2023

unfortunately I mixed in the renaming of ImportedSource to ImportedCallable into this PR and it's counterpart in spec: bioimage-io/spec-bioimage-io#478

It's just some renaming to avoid confusion about source as in source data and source as in a python callable defined in a module or provided source code.

@FynnBe FynnBe marked this pull request as ready for review February 9, 2023 12:32
@FynnBe
Copy link
Member Author

FynnBe commented Feb 13, 2023

together with new repo https://github.com/bioimage-io/workflows-bioimage-io-python this updated PR supersedes #312 and #316

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

Successfully merging this pull request may close these issues.

2 participants