-
Notifications
You must be signed in to change notification settings - Fork 18
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
Deprecate the use of ipynbname package in our SDK #184
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.
The change looks good to me! But we definitely need an integration test for when the function is in a python file in a different directory, and it makes a relative import from another directory (this may already exist but would be good to check)
Would we be able to modify one of our example notebooks to include an operator defined in a separate python file (with additional dependencies in separate files too)? We can also just create a simple example test notebook and check it with the run-notebook
action.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@kenxu95 could you take another look? I added a notebook integration test. |
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.
LGTM!
Describe your changes and why you are making these changes
The
ipynbname
has been causing issues whenever a notebook isn't started in the user's own terminal. This PR removes the use of this package and also clean up some deprecated logic in the packaging function. Please see the comments inline for the defensive check added.Related issue number (if any)
Checklist before requesting a review