-
Notifications
You must be signed in to change notification settings - Fork 916
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
[skip-ci] Updates to 10min notebook #10531
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Just a few ideas for future improvement - this looks fine if you want to merge as-is.
"name": "stderr", | ||
"output_type": "stream", | ||
"text": [ | ||
"/home/ashwin/workspace/rapids/cudf/python/cudf/cudf/core/indexed_frame.py:2271: FutureWarning: append is deprecated and will be removed in a future version. Use concat instead.\n", |
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.
@shwina Should we fix/remove the cell input that generated this warning, or possibly remove the user path from the output?
I wrote a pre-commit hook called nb-strip-paths before joining NVIDIA that can help prevent user paths from being committed to notebooks (MIT license, but no existing SWIPAT). It would convert /home/ashwin/workspace/rapids/cudf/python/cudf/cudf/core/indexed_frame.py
to python/cudf/cudf/core/indexed_frame.py
if run on your local machine when committing.
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.
That's cool! This would be nice to have. But probably a bit late in the release for right now.
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.
Yup! I agree. Just wanted to mention it for the future.
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.
We still might want to fix the cell input that generated the warning.
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.
Fix'd
" </tr>\n", | ||
" <tr>\n", | ||
" <td colspan=\"2\" style=\"text-align: left;\">\n", | ||
" <strong>Local directory: </strong> /home/ashwin/workspace/rapids/cudf/docs/cudf/source/user_guide/dask-worker-space/worker-0qm0cp4w\n", |
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.
Lines like this would also be filtered through nb-strip-paths. Result path should be docs/cudf/source/user_guide/dask-worker-space/worker-0qm0cp4w
.
"name": "stderr", | ||
"output_type": "stream", | ||
"text": [ | ||
"/tmp/ipykernel_3442/3638222946.py:7: DeprecationWarning: an integer is required (got type cupy._core.core.ndarray). Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.\n", |
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.
Might be nice to fix this warning too.
This PR closes #10527, an issue with outdated code in our "10 minutes" notebook. The material changes are on lines 65 and 66.