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

Deprecate future package and drop Python2 support #2082

Merged

Conversation

namannandan
Copy link
Collaborator

@namannandan namannandan commented Jan 19, 2023

Description

Remove dependence on the future package and references to Python2 in order to resolve security vulnerabilities.

Unregister resnet-18 model after testing load from snapshot in sanity test to avoid subsequent sanity runs from failing.

Fixes #(2063)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

(ts-conda-test) $ which torchserve
/home/ubuntu/anaconda3/envs/ts-conda-test/bin/torchserve
(ts-conda-test) $ which torch-model-archiver
/home/ubuntu/anaconda3/envs/ts-conda-test/bin/torch-model-archiver
(ts-conda-test) $ which torch-workflow-archiver
/home/ubuntu/anaconda3/envs/ts-conda-test/bin/torch-workflow-archiver
(ts-conda-test) $ pip freeze | grep future
(ts-conda-test) $ python torchserve_sanity.py &> conda_bin_sanity_log.txt

CPU: cpu_conda_bin_sanity_log.txt
GPU: gpu_conda_bin_sanity_log.txt

The conda packages were verified by running the test_torchserve, test_modelarchiver, test_workflow_archiver, test_sanity and test_workflow_sanity methods in the torchserve_sanity script

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #2082 (b80f783) into master (8ace3c0) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2082      +/-   ##
==========================================
- Coverage   53.38%   53.36%   -0.02%     
==========================================
  Files          71       71              
  Lines        3224     3225       +1     
  Branches       56       56              
==========================================
  Hits         1721     1721              
- Misses       1503     1504       +1     
Impacted Files Coverage Δ
setup.py 0.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@namannandan namannandan marked this pull request as ready for review January 19, 2023 21:50
@lxning
Copy link
Collaborator

lxning commented Jan 20, 2023

@namannandan please also add test log for gpu.

@@ -127,8 +127,6 @@ public void setEnvelope(String envelope) {
public enum RuntimeType {
@SerializedName("python")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@namannandan Could you please clarify if python here and in subsequent files in java means python3?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we drop support for Python 2, would it make sense to consolidate the python runtimes into one and rely on the configuration to figure out the right executable?

pythonRuntime = configManager.getPythonExecutable();

Copy link
Collaborator Author

@namannandan namannandan Jan 23, 2023

Choose a reason for hiding this comment

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

@agunapal here, python refers to the system default python binary and would not guarantee that it is aliased to a python3 binary. That would depend on how the environment in which Torchserve is running has been set up. In spite of this, I think there may be use-cases where this could be helpful. Please see my comment below and let me know your thoughts.

@mreso yes, that makes sense to me.
Although, I was wondering if a use-case with the following environment would benefit from having support for both python and python3 runtime options:

  • python -> points to the system default python version (say python 3.8)
  • python3 -> points to a more recent release of python3 (say python 3.11)

This could give choice between the different python 3.x releases without having to change what python points to. Let me know your thoughts on if this is useful. If not, I can go ahead and remove the reference to python3 as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@namannandan yeah, I see your point, though it would look like a hacked together solution and not like a feature to me. Making runtimes configurable and extendable would probably be my preference but there is probably little gain for users out of the non-negligible effort.

Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

Besides the remaining question if we should consolidate runtimes LGTM

@@ -127,8 +127,6 @@ public void setEnvelope(String envelope) {
public enum RuntimeType {
@SerializedName("python")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we drop support for Python 2, would it make sense to consolidate the python runtimes into one and rely on the configuration to figure out the right executable?

pythonRuntime = configManager.getPythonExecutable();

@@ -58,7 +58,7 @@ torch-model-archiver --model-name densenet161 --version 1.0 --model-file example
$ torch-model-archiver -h
usage: torch-model-archiver [-h] --model-name MODEL_NAME --version MODEL_VERSION_NUMBER
--model-file MODEL_FILE_PATH --serialized-file MODEL_SERIALIZED_PATH
--handler HANDLER [--runtime {python,python2,python3}]
--handler HANDLER [--runtime {python,python3}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a period of time we could just map python3 to python with a deprecation warning and instruction to select executable through config.

@namannandan
Copy link
Collaborator Author

@namannandan please also add test log for gpu.

Updated summary to include test logs for GPU.

@namannandan
Copy link
Collaborator Author

namannandan commented Jan 24, 2023

Will go ahead and merge this PR since it addresses the major concern about removing the future package. Further improvements to consolidating the runtime options could be made in a future PR.

@namannandan namannandan merged commit 7a4cb4d into pytorch:master Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants