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

use 'sagemaker-python-sdk' instead of 'sagemaker' #504

Merged
merged 14 commits into from
Feb 13, 2025

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Feb 5, 2025

Fixes #443
Contributes to #506

Notes for Reviewers

I tested the SageMaker pages as part of this:

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Feb 5, 2025
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

- **IAM_ROLE** = Create a new role > Create role
- **Notebook instance name** = Name of the notebook instance
- **Notebook instance type** = Type of notebook instance. Select a RAPIDS-compatible GPU ([see the RAPIDS docs](https://docs.rapids.ai/install#system-req)) as the SageMaker Notebook instance type (e.g., `ml.p3.2xlarge`).
- **Platform identifier** = 'Amazon Linux 2, Jupyter Lab 4'
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes here:

  • use the same case as the UI
  • remove "IAM Role" (these examples work with the default role added by SageMaker)
  • update to Amazon Linux 2, Jupyter Lab 4 (the latest platform SageMaker supports)

@@ -1,27 +0,0 @@
ARG RAPIDS_IMAGE
Copy link
Member Author

Choose a reason for hiding this comment

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

There is code in the notebook that writes this... it's not necessary to have a copy checked into the repo too.

" cupy \\\n",
" dask-ml \\\n",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was missing, and so the example code failed like this:

Traceback (most recent call last):
  File "/opt/ml/code/train.py", line 75, in <module>
    train()
  File "/opt/ml/code/train.py", line 27, in train
    ml_workflow = create_workflow(hpo_config)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/ml/code/MLWorkflow.py", line 43, in create_workflow
    from workflows.MLWorkflowMultiGPU import MLWorkflowMultiGPU
  File "/opt/ml/code/workflows/MLWorkflowMultiGPU.py", line 34, in <module>
    from dask_ml.model_selection import train_test_split
ModuleNotFoundError: No module named 'dask_ml'

" flask \\\n",
" protobuf \\\n",
" sagemaker\n",
" rapids-dask-dependency=${{ rapids_version }} \\\n",
Copy link
Member Author

Choose a reason for hiding this comment

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

To ensure dask-ml installation doesn't result in upgrading / downgrading dask and distributed and therefore changing the installed version of RAPIDS libraries.

},
"nightly": {
"rapids_version": f"{nightly_version}-nightly",
"rapids_version": f"{nightly_version}",
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing in the repo relies on this -nightly being there, as far as I can tell.

git grep rapids_version

And in fact, this difference is causing some bugs. Look at https://docs.rapids.ai/deployment/nightly/platforms/databricks/#install-rapids-and-dask .... it's saying you should install dask-cuda==25.02-nightly, which does not exist.

Screenshot 2025-02-11 at 9 00 42 PM

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this might be a holdover from a previous naming scheme. Fine to remove it.

@jameslamb jameslamb changed the title WIP: use 'sagemaker-python-sdk' instead of 'sagemaker' use 'sagemaker-python-sdk' instead of 'sagemaker' Feb 12, 2025
@jameslamb jameslamb marked this pull request as ready for review February 12, 2025 03:01
@jameslamb jameslamb requested a review from a team as a code owner February 12, 2025 03:01
@jameslamb jameslamb mentioned this pull request Feb 12, 2025
65 tasks
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This looks good!

Could we add a warning admonition that explains to users that we need to pin until Sagemaker moves off Amazon Linux 2?

},
"nightly": {
"rapids_version": f"{nightly_version}-nightly",
"rapids_version": f"{nightly_version}",
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this might be a holdover from a previous naming scheme. Fine to remove it.

RAPIDS `>24.12` will not be installable on SageMaker Notebook Instances until those instances support
Amazon Linux 2023 or other Linux distributions with GLIBC of at least 2.28.
For more details, see https://github.com/rapidsai/deployment/issues/520.
```
Copy link
Member Author

Choose a reason for hiding this comment

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

@jacobtomlinson what do you think about this language for a warning?

I checked the preview and looks like it's rendering OK:

image

https://rapids-deployment--504.org.readthedocs.build/en/504/cloud/aws/sagemaker/

Copy link
Member

Choose a reason for hiding this comment

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

Language looks good 👍

@jacobtomlinson jacobtomlinson merged commit 45d97dc into rapidsai:main Feb 13, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AWS] RAPIDS 24.10/24.12 and the sagemaker library can't be installed together
2 participants