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

Add tests for simple graph, with multiple inputs #1018

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Conversation

stangirala
Copy link
Collaborator

@stangirala stangirala commented Nov 11, 2024

Context

Add a couple of tests for a simple graph function where the function takes in multiple inputs. This should work given how we setup the args but we didn't have a test for it. Also add test, for sanity, with json encoding.

What

Add tests for graph behaviours. This will run as a workflow action.

Testing

N/A

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.

Copy link
Collaborator

@diptanu diptanu left a comment

Choose a reason for hiding this comment

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

Left some comments, merge it once the tests are green!

@@ -208,6 +215,18 @@ def test_simple_function(self, is_remote):
output = graph.output(invocation_id, "simple_function")
self.assertEqual(output, [MyObject(x="ab")])

# @parameterized.expand([(False), (True)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably shouldn't be commented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ran into this on my laptop testing,ModuleNotFoundError: No module named 'test_graph_behaviours'. I just pushed the PR to run the test and see if it worked. Will fix.

@@ -208,6 +215,18 @@ def test_simple_function(self, is_remote):
output = graph.output(invocation_id, "simple_function")
self.assertEqual(output, [MyObject(x="ab")])

# @parameterized.expand([(False), (True)])
def test_simple_function_multiple_inputs(self, is_remote=True):
print(sys.version_info.major, sys.version_info.minor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unintented?

@@ -28,6 +29,12 @@ def simple_function(x: MyObject) -> MyObject:
return MyObject(x=x.x + "b")


@indexify_function()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably worth testing if this works with json encoding as well. May be it works.

@stangirala stangirala changed the title TEST Add test for simple graph, with multiple inputs Add tests for simple graph, with multiple inputs Nov 11, 2024
@stangirala stangirala merged commit f04a883 into main Nov 11, 2024
5 checks passed
@stangirala stangirala deleted the multiple_input_test branch November 11, 2024 06:00
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