change: makes pandas optional #4222
Conversation
There was a problem hiding this comment.
Thank you, @trungleduc.
Let's see if some configurations tested on the CI break with this change.
In the meantime, here is a first review.
| def is_likely_a_pandas_df(data: Any) -> bool: | ||
| """Return True if data is likely a pandas dataframe.""" | ||
| return ( | ||
| data.__class__.__name__ == "DataFrame" | ||
| and hasattr(data, "columns") | ||
| and hasattr(data, "iloc") | ||
| ) |
There was a problem hiding this comment.
I think we must test this utility function.
We could use pytest.importorskip in the test:
pd = pytest.importorskip("pandas")| try: | ||
| import pandas as pd | ||
| except ImportError as e: | ||
| logger.warning("pandas failed to import. Feature store features will be impaired or broken.") |
There was a problem hiding this comment.
Would being explicit help in this case?
| logger.warning("pandas failed to import. Feature store features will be impaired or broken.") | |
| logger.warning( | |
| "pandas is not installed in your environment. " | |
| "Feature store features will be impaired or broken." | |
| ) |
This suggestion also applies to other warning messages.
There was a problem hiding this comment.
I want to keep it consistent with other messages of the SDK and also this message reflects correctly the import issue
There was a problem hiding this comment.
Can we also add an instruction to install from extras, would it help for users to unblock when hit with this issue.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4222 +/- ##
==========================================
- Coverage 90.27% 90.26% -0.01%
==========================================
Files 993 320 -673
Lines 87861 29752 -58109
==========================================
- Hits 79313 26855 -52458
+ Misses 8548 2897 -5651
☔ View full report in Codecov by Sentry. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -56,8 +56,8 @@ def read_requirements(filename): | |||
| "smdebug_rulesconfig==1.0.1", | |||
| "importlib-metadata>=1.4.0,<7.0", | |||
| "packaging>=20.0", | |||
| "pandas", | |||
There was a problem hiding this comment.
This I think will be a breaking change, customers who auto consume newest pysdk release with this fix would now need to also install pandas (in case they use it). But I do still think this is a valuable change considering the use-cases of supporting lambda.
There was a problem hiding this comment.
Can we try this approach where we can provide documentation for minimal installation like:
pip install --no-deps sagemaker
pip install -r minimal_requirements.txt // this has the minimal requirements without pandas.
we can put this in a shell script and call it lambda-install.sh or something and put it in sdk,
There was a problem hiding this comment.
There is no easy way to provide the minimal_requirements.txt file to users, since installing the SDK with pip only copies the Python files to the install path, not the requirement files. And it's the same situation with an install script
There was a problem hiding this comment.
Do you want to include requirement files in the Pypi package?
There was a problem hiding this comment.
is there a way to do it? the only way to solve the issue in a backwards compatible way is to provide documentation to users for AWS Lambda support with no-deps instructions above.
There was a problem hiding this comment.
if its not possible to include files in pip dist, lets just provide adequate documentation to users.
There was a problem hiding this comment.
Yes, it's possible
There was a problem hiding this comment.
The requirement files are not stored in the source folder of the SDK, so shipping them with the SDK is not recommended. I went with the documentation route.
There was a problem hiding this comment.
| try: | ||
| import pandas as pd | ||
| except ImportError as e: | ||
| logger.warning("pandas failed to import. Feature store features will be impaired or broken.") |
There was a problem hiding this comment.
Can we also add an instruction to install from extras, would it help for users to unblock when hit with this issue.
setup.py
Outdated
| "pathos", | ||
| "pytz", |
There was a problem hiding this comment.
do we need this new dependency, how much of a file size addition is this? if needed can we rely on existing dependencies (/some dependency from python standard library)
There was a problem hiding this comment.
it's used in the code base but is installed implicitly through pandas. I wanted to address it in a separate PR. But can also do it here
There was a problem hiding this comment.
Yeah we should really really careful while adding new dependency and only do it if absolutely necessary, otherwise use extras [added dependency]
There was a problem hiding this comment.
Pandas < 2.0 necessitates pytz but Pandas>=2.0 now uses datetime.timezone by default.
Probably pytz was introduced when installing Pandas < 2.0.
If we depends on Pandas 2.0, we can safely remove pytz as a constraint.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
pandasby default but won't raise any error without it. Users can install the SDK without pandas by following the guide in the documentation https://sagemaker--4222.org.readthedocs.build/en/4222/installation.htmlDescription of changes:
try-exceptblock withDeferredErrorDataFrameis imported underTYPE_CHECKINGcheckis_likely_a_pandas_dfto heuristically check if the variable is aDataFrameTesting done:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.