-
Notifications
You must be signed in to change notification settings - Fork 74
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
notebook integration: %burr_ui
can show the web app in an iframe
#430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to fdc7e00 in 16 seconds
More details
- Looked at
77
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. burr/integrations/notebook.py:17
- Draft comment:
The check forIPKernelApp
in the Jupyter environment is redundant here and should be moved to the end as a fallback, which is already done in the PR. - Reason this comment was not posted:
Confidence changes required:0%
Theidentify_notebook_environment
function has a redundant check forIPKernelApp
in the Jupyter environment. This check should be moved to the end as a fallback, which is already done in the PR.
2. burr/integrations/notebook.py:144
- Draft comment:
Consider handling the case where the subprocess fails to launch in theburr_ui
method, potentially by providing a user-friendly error message. - Reason this comment was not posted:
Confidence changes required:50%
Theburr_ui
method in theNotebookMagics
class should handle the case where the subprocess fails to launch, potentially by providing a user-friendly error message.
Workflow ID: wflow_IY42QiTNFuZuHLd7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
A preview of e87a696 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/430 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
if args.no_iframe is True: | ||
print(f"Burr UI: {self.url}") | ||
else: | ||
display(IFrame(self.url, width="100%", height=args.height)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the default is to show it, but you can disable it?
@zilto just need to re-run pre-commit and fix what it wants so it passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on e87a696 in 17 seconds
More details
- Looked at
23
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. burr/integrations/notebook.py:127
- Draft comment:
Consider documenting the default value of 400 in the help description for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The--height
argument default value is set to 400, which might not be suitable for all use cases. It would be better to document this default value in the help description for clarity.
Workflow ID: wflow_ApdoAhzJRFIDsLvL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Adds the arguments
--no-iframe
(default is enabling iframe) and--height
.Notes:
100%
so there's notwidth
arg%burr_ui
multiple times will check if the subprocess exited before retrying. If there's a bug in launching the UI, the user may repeatedly hit "launch"