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

Build with Local Function Directly #793

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

Conversation

swapdewalkar
Copy link
Contributor

@swapdewalkar swapdewalkar commented Mar 28, 2024

--- PR TEMPLATE INSTRUCTIONS (1) ---

Address this Issue #685

Changes

  • Added a local builder helper.
  • Used python inspect to find out parent module and included with in sys modules.

How I tested this

Unit Tests

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@skrawcz
Copy link
Collaborator

skrawcz commented Mar 28, 2024

@swapdewalkar can you motivate this with an example please? I'm interested, curious to hear where/when you'd use it.

@swapdewalkar
Copy link
Contributor Author

@skrawcz I saw the requirement here to get all fns that are defined in the local module and insert them into a driver.

dr = driver.Builder()
      .with_local_modules()
      .build()

instead of

if __name__ == '__main__':
    import __main__
    dr = driver.Builder()
                     .with_modules(__main__)
                     .build()
    print(dr.execute(["foo"]))

Let me know if I understood something different from ticket.

Comment on lines +3 to +7

def test_driver_with_local_modules() -> None:
dr = Builder().with_local_modules().build()
assert isinstance(dr.graph_executor, DefaultGraphExecutor)
assert __name__ == list(dr.graph_modules)[0].__name__
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this test probably needs to pull in a function and exercise the driver.

e.g. define

def a(b:int) -> int:
   return b * 2

Then we should execute the driver to get "a".

That should work right?

@skrawcz
Copy link
Collaborator

skrawcz commented Apr 1, 2024

@swapdewalkar cool makes sense -- want to comment on that ticket so I can assign it to you?

Otherwise I think we just need a test to exercise this properly.

@zilto
Copy link
Collaborator

zilto commented Apr 1, 2024

@swapdewalkar thanks for the contribution!

The Python import system behaves in strange ways at times, so it would be beneficial if you could add to the docstring of .with_local_modules() a snippet of what Python code should be in your .py file and how to execute the code.

For instance, the current approach with inspect.stack()[1][0] seems to work for python -m run but will fail for python run.py of the same file. In that case, it can lead to hard to debug IndexError: list index out of range

A potential solution would be

def with_local_modules(self) -> "Builder":
    """Adds the local modules to the modules list.
    :return: self
    """
    module = __import__("__main__")
    self.modules.append(module)
    return self

This will get the module "__main__" from sys.modules and should be directly equivalent to

if __name__ == "__main__":
  import __main__

But I invite you to investigate and set the appropriate tests!

"""
import inspect

module = inspect.getmodule(inspect.stack()[1][0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

See Thierry's comment. I think we can make this more robust.

@@ -1669,6 +1669,17 @@ def with_config(self, config: Dict[str, Any]) -> "Builder":
self.config.update(config)
return self

def with_local_modules(self) -> "Builder":
"""Adds the local modules to the modules list.
Copy link
Collaborator

@skrawcz skrawcz Apr 2, 2024

Choose a reason for hiding this comment

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

We should add more caveats here.
E.g. If people use this, they could run into problems when using some of the adapters because this code will be imported under the module __main__.

@swapdewalkar
Copy link
Contributor Author

I will add more tests!

@skrawcz
Copy link
Collaborator

skrawcz commented Apr 22, 2024

Just waiting on more tests for this one :)

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