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

fix(sdk): fix support for args when using IndexifyFunction and IndexifyRouter classes #1127

Merged
merged 3 commits into from
Dec 20, 2024

Conversation

seriousben
Copy link
Member

@seriousben seriousben commented Dec 19, 2024

Context

We introduced a regression making this graph fail:

        class MyObject(BaseModel):
            x: int

        class SimpleFunctionCtxCls(IndexifyFunction):
            name = "SimpleFunctionCtxCls"

            def __init__(self):
                super().__init__()

            def run(self, obj: MyObject) -> MyObject:
                return MyObject(x=obj.x + 1)

        graph = Graph(name="test_simple_function_cls", start_node=SimpleFunctionCtxCls)
        graph = remote_or_local_graph(graph, is_remote)
        invocation_id = graph.run(block_until_done=True, obj=MyObject(x=1))
        output = graph.output(invocation_id, "SimpleFunctionCtxCls")
        self.assertEqual(output, [MyObject(x=2)])

It failed because it called the run function with dict(obj=MyObject(x=1)) instead of just MyObject(x=1).

What

Recent improvements to args and kwargs handling missed support for IndexifyFunction and IndexifyRouter classes.

This PR aligns the approach taken for function and classes.

Testing

Add a new test case for IndexifyFunction and IndexifyRouter classes.

Contribution Checklist

  • If the python-sdk was changed, please run make fmt in python-sdk/.
  • If the server was changed, please run make fmt in server/.
  • Make sure all PR Checks are passing.

@stangirala
Copy link
Collaborator

+1 (needs pr check fix)

@seriousben seriousben force-pushed the seriousben/fix-args-support-for-class-functions branch from 11affaf to ace67cc Compare December 20, 2024 01:01
return str(
type(func_wrapper.indexify_function)
) == "<class 'indexify.functions_sdk.indexify_functions.IndexifyRouter'>" or isinstance(
func_wrapper.indexify_function, IndexifyRouter
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we only use the isinstance check here?
It's clear easier to maintain if it gives the same results.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my testing, the way we build the IndexifyFunction using type(...) fails the is instance check.

Will do more testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. If isinstance() fails and we won't figure out why, then let's not add isinstance() here and add a comment that's it's not working. So others don't try to quickfix it with the isinstance() check.

Copy link
Member Author

@seriousben seriousben Dec 20, 2024

Choose a reason for hiding this comment

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

I would not be adding code for no reason :)

The isintance is needed when using the IndexifyRouter class. But it looks like when using the indexify_router function, it is not enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it can either be an instance or a class.

@seriousben seriousben merged commit 501be11 into main Dec 20, 2024
8 checks passed
@seriousben seriousben deleted the seriousben/fix-args-support-for-class-functions branch December 20, 2024 15:52
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.

3 participants