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

Jupyter magics v2 #846

Closed
wants to merge 6 commits into from
Closed

Jupyter magics v2 #846

wants to merge 6 commits into from

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Apr 25, 2024

Please read the tutorial notebook for all details.

TODO

  • do some light refactoring to allow a barebone %%execute cell magic
  • convert the jupyter notebook to markdown and add it to docs

Changes

  • this currently overrides the previous jupyter magics and isn't backward compatible
  • To make things backward compatible, we can use a different filename jupyter_extensions to be consistent with other extensions? Nothing stops this file from containing other things than "magics". This matters because it's what users need to import via %load_ext hamilton.plugins.jupyter_extensions whereas "magic" is probably an odd term for most people

How I tested this

  • it's hard to test given the "user interface" nature of the tool. Most of it is parsing code cells + dynamic module importing + standard Hamilton
  • the main new feature is rebuild_driver()

Notes

  • I see to get performance hit after 50+ cell execution (maybe my computer is behaving oddly). This maybe because each time we create an ad hoc module, we add it to sys.modules? Maybe it's just VSCode + Pylance LSP disliking magic code cells. Don't know

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.

@zilto zilto added enhancement New feature or request examples Related to code under `examples/` labels Apr 25, 2024
Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Not 100% sure the compatibility case. It's backwards-incompatible, right? If so, are we OK with that?

hamilton/plugins/jupyter_magic.py Outdated Show resolved Hide resolved
hamilton/plugins/jupyter_magic.py Outdated Show resolved Hide resolved
hamilton/plugins/jupyter_magic.py Show resolved Hide resolved
Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

yeah so I like some of the changes, but not others.

For example the departure from having to provide valid driver builder code is a concern because I think that makes the experience confusing from regular Hamilton. It's also not clear where the modules comes from that go into the driver. E.g. What if I have multiple modules and want several different drivers?

Let's chat about it some more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we link to this file from the blogs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will create a new example.ipynb based on the recent RAG PR once we settle on the Jupyter V2 API

@zilto zilto requested a review from skrawcz April 27, 2024 20:47
Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

yeah so out of principal we don't push backwards incompatible changes.

Options:

  • rename the magic.
  • live with -m ...
  • have a really good error message and suggest the fix and apologize and ask for forgiveness... (is this possible to do well?)

@skrawcz
Copy link
Collaborator

skrawcz commented May 13, 2024

From chatting:

  1. restart from main.
  2. we want to make sure things are backwards compatible -- add deprecation warnings/etc.


```python
%%cell_to_module -m MODULE_NAME # more args
Copy link
Collaborator

Choose a reason for hiding this comment

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

should have output of ?cell_to_module instead.

@zilto
Copy link
Collaborator Author

zilto commented May 23, 2024

Closing in favor of #917

@zilto zilto closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request examples Related to code under `examples/`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants