Skip to content

Conversation

@zilto
Copy link
Contributor

@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
Contributor

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

return nodes


def create_function_graph(
Copy link
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor 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
Contributor

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
Contributor

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.

@zilto
Copy link
Contributor Author

zilto commented Feb 7, 2025

closing in favor of #1275

@zilto zilto closed this Feb 7, 2025
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