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

[WIP] feat: build FunctionGraph from functions #1271

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

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Jan 13, 2025

Changes

  • The changes affect core code path, but is generally safe. Only outdated tests using graph.create_function_graph() instead of graph.FunctionGraph.from_modules() needed to be updated.
  • The function signature of graph.create_function_graph() was modified, but it was never advertised as a public API. Further rewiring could provide full backwards compatibility (could do after validating the general solution)

TODO

  • update documentation
  • change create_function_graph() for fully backwards-compatible APi

How I tested this

  • all tests pass after slight edits

Notes

  • graph.create_function_graph() should really return a FunctionGraph instead of a dict

@zilto zilto added the core-work Work that is "core". Likely overseen by core team in most cases. label Jan 13, 2025
ellipsis-dev[bot]

This comment was marked as spam.

ellipsis-dev[bot]

This comment was marked as spam.

@zilto zilto changed the title feat: build FunctionGraph from functions [WIP] feat: build FunctionGraph from functions Jan 13, 2025
@elijahbenizzy
Copy link
Collaborator

Hey -- sorry, it's been a bit, will be looking over soon!

@@ -143,7 +143,7 @@ def update_dependencies(


def create_function_graph(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The type signature of this should be updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also 🤦 (at myself I think) that the name of this function and what it returns - a dict, not a function graph...

@skrawcz
Copy link
Collaborator

skrawcz commented Jan 31, 2025

Yeah so what's left? This ?

  • add in ability to pass functions and update code to handle that?
  • add tests for new functionality
  • add example

@skrawcz
Copy link
Collaborator

skrawcz commented Jan 31, 2025

My expectation

# module Z
# some funcs

# module 1
def func1( ...) -> ...:
  ...

# module 1, or perhaps another module 2
def func2(...)->...:
  ...

# driver code somewhere else or in one of the modules - importing things appropriately
funcs = [func1, func2] 

driver.Builder().with_modules(module_z, *funcs)... # this should work

it should also work with .allow_module_overrides() on the driver Builder, thus allowing one to inject a new function implementation.

@leblancfg
Copy link

I saw the note in #1271:

Benefits
facilitate marimo integration

FYI I attempted this with this branch, but it returns TypeError: unhashable type: 'Cell'.

@zilto
Copy link
Collaborator Author

zilto commented Feb 3, 2025

I saw the note in #1271:

Benefits
facilitate marimo integration

FYI I attempted this with this branch, but it returns TypeError: unhashable type: 'Cell'.

@leblancfg You're ahead of us! Will open a separate PR and share my marimo development pattern once this is merged. Feel free to discuss here #1274

@elijahbenizzy
Copy link
Collaborator

OK, with_modules(...) shouldn't accept functions. with_function(...) should, IMO.

That said -- in order to have function overrides, we should probably want to combine from_fucntions and from_modules into construct(...) or compile(...) that takes in a list of functions/modules. Happy to take a stab! Should be straightforward -- we can even leave around the old one for backwards compatibility.

@skrawcz
Copy link
Collaborator

skrawcz commented Feb 4, 2025

OK, with_modules(...) shouldn't accept functions. with_function(...) should, IMO.

Disagree. I think this will make things more confusing, as it will:

  1. create two ways to create nodes in the graph.
  2. provide no clear precedence when say someone wants to override a function in a module.

I think extending with_modules is fine. It also makes it very clear the order of application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-work Work that is "core". Likely overseen by core team in most cases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants