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

fix(batch-predictor, pyfunc-server): Pin setuptools max version #608

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

deadlycoconuts
Copy link
Contributor

Description

It seems like from the 20th of September 2024, CICD tasks that require building a python environment started failing with the following error when the ancient mlflow package tries to log artifacts:

ModuleNotFoundError: No module named 'distutils._modified'

This seems to be an error reported recently by other people in the pyinstaller repo, with some users mentioning that distutils is used by setuptools, which just had a new version released recently. Since it doesn't seem like we're importing distutils directly, I've decided to simply pin down the max version of setuptools to <75. Note that the version of setuputils somehow needs to be specified in the variable passed to this argument install_requires here

install_requires=REQUIRE,

As such, I've added setuptools<75 in both the batch predictor and pyfunc server's requirements.txt file.

Note that this PR technically does not fix the root cause of the problem; it's only a band-aid solution for the moment until we migrate to newer versions of Mlflow very shortly. When that happens I'll revert the changes in this PR.

Modifications

  • .github/workflows/release.yml - Updated the versions of setuptools in the build jobs for the python-related components
  • python/batch-predictor/requirements.txt - Added setuptools<75 to the requirements of the batch predictor
  • python/pyfunc-server/requirements.txt - Added setuptools<75 to the requirements of the pyfunc server

Tests

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes

NONE

@deadlycoconuts deadlycoconuts added the bug Something isn't working label Sep 20, 2024
@deadlycoconuts deadlycoconuts self-assigned this Sep 20, 2024
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.70%. Comparing base (a9184ed) to head (4ca6418).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #608   +/-   ##
=======================================
  Coverage   60.70%   60.70%           
=======================================
  Files         275      275           
  Lines       22171    22171           
=======================================
  Hits        13460    13460           
  Misses       7847     7847           
  Partials      864      864           
Flag Coverage Δ
api-test 58.68% <ø> (ø)
sdk-test-3.10 75.51% <ø> (ø)
sdk-test-3.8 75.49% <ø> (ø)
sdk-test-3.9 75.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@tiopramayudi tiopramayudi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @deadlycoconuts

@deadlycoconuts deadlycoconuts marked this pull request as ready for review September 23, 2024 04:25
@deadlycoconuts
Copy link
Contributor Author

deadlycoconuts commented Sep 23, 2024

Thanks for the quick review @tiopramayudi! I'll merge this now!

@deadlycoconuts deadlycoconuts merged commit 2cb5e3b into main Sep 23, 2024
36 checks passed
@deadlycoconuts deadlycoconuts deleted the pin_max_setuptools_version branch September 23, 2024 04:26
deadlycoconuts added a commit that referenced this pull request Sep 23, 2024
# Description
In PR #608, the version of `setuptools` was pinned to `<75`. However,
this wasn't done correctly for the pip installation command, causing the
CICD pipeline to
[fail](https://github.com/caraml-dev/merlin/actions/runs/10988775479/job/30506834078#step:4:13)
when publishing new versions of the Merlin SDK, batch predictor and
pyfunc server to PyPI. This PR simply corrects that invalid command.

# Modifications
- `.github/workflows/release.yml` - Fix incorrect pip installation
command

# Tests
<!-- Besides the existing / updated automated tests, what specific
scenarios should be tested? Consider the backward compatibility of the
changes, whether corner cases are covered, etc. Please describe the
tests and check the ones that have been completed. Eg:
- [x] Deploying new and existing standard models
- [ ] Deploying PyFunc models
-->

# Checklist
- [x] Added PR label
- [ ] Added unit test, integration, and/or e2e tests
- [ ] Tested locally
- [ ] Updated documentation
- [ ] Update Swagger spec if the PR introduce API changes
- [ ] Regenerated Golang and Python client if the PR introduces API
changes

# Release Notes
```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants