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

Replace nvtabular.utils with merlin.core.compat #1795

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

edknv
Copy link
Contributor

@edknv edknv commented Apr 3, 2023

Same change as NVIDIA-Merlin/Merlin#892.

There was a change in Core that moved pynvml_mem_size from merlin.core.utils to merlin.core.compat. This PR replaces nvtabular.utils with merlin.core.compat in examples/03-Running-on-multiple-GPUs-or-on-CPU.ipynb.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@edknv edknv self-assigned this Apr 3, 2023
@edknv edknv modified the milestones: Merlin 23.03, Merlin 23.04 Apr 3, 2023
@rnyak rnyak requested a review from karlhigley April 3, 2023 18:24
Copy link
Contributor

@karlhigley karlhigley left a comment

Choose a reason for hiding this comment

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

Wonder if nvtabular.utils should import merlin.core.compat.pynvml_mem_size in order to work around the breaking change in Core

@edknv
Copy link
Contributor Author

edknv commented Apr 3, 2023

It looks like we've meant to deprecate nvtabular.utils and move it to Core for a while:

"The `nvtabular.utils` module has moved to `merlin.core.utils`. "
"Support for importing from `nvtabular.utils` is deprecated, "
"and will be removed in a future version. Please update "
"your imports to refer to `merlin.core.utils`.",

@jperez999 jperez999 merged commit 8ad2df6 into NVIDIA-Merlin:main Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants