Model archiver api #2751

GeeCastro · 2023-10-30T23:14:21Z

Description

Adds a model archiver interface to archive models programmatically with a config class ModelArchiverConfig and an archiver class ModelArchiver. Simply wraps the existing CLI feature.

Fixes #2695

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
New feature (non-breaking change which adds functionality)
This change requires a documentation update

Feature/Issue validation/testing

As it's only wrapping the actual feature there's only a unit test very similar to the existing packaging test.

Test Model archiver

model_archiver/tests/unit_tests/test_model_archiver.py::TestModelArchiver::test_gen_model_archive PASSED                                                     [  3%]

Test Model archiver config from argparse

model_archiver/tests/unit_tests/test_model_archiver.py::TestModelArchiver::test_model_archiver_config_from_args PASSED                                       [  6%]

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? Not yet
Have you made corresponding changes to the documentation? Not yet

msaroufim · 2023-10-30T23:50:42Z

Very nice change, wanna create a new page something like documentation/python_api.md so I can evangelize this change to others. I'd be excited about doing this as well for torchserve --start and eventually all our REST APIs as well

codecov · 2023-10-30T23:57:05Z

Codecov Report

Merging #2751 (5d702e2) into master (ada7856) will increase coverage by 0.22%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 5d702e2 differs from pull request most recent head 3e3609c. Consider uploading reports for the commit 3e3609c to get more accurate results

@@            Coverage Diff             @@
##           master    #2751      +/-   ##
==========================================
+ Coverage   72.44%   72.66%   +0.22%     
==========================================
  Files          85       87       +2     
  Lines        3963     3995      +32     
  Branches       58       60       +2     
==========================================
+ Hits         2871     2903      +32     
  Misses       1088     1088              
  Partials        4        4

Files	Coverage Δ
model-archiver/model_archiver/model_archiver.py	`100.00% <100.00%> (ø)`
...l-archiver/model_archiver/model_archiver_config.py	`100.00% <100.00%> (ø)`
model-archiver/model_archiver/model_packaging.py	`100.00% <100.00%> (ø)`
...l-archiver/model_archiver/model_packaging_utils.py	`92.35% <100.00%> (+0.04%)`	⬆️

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

GeeCastro · 2023-10-31T23:16:04Z

model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py

        patches.export_utils.validate_inputs.assert_called()
        patches.export_utils.archive.assert_called()

    def test_export_model_method_tar(self, patches):
-        self.args.update(archive_format="tar")
+        self.config.archive_format = "tar"


This value "tar" doesn't align with the list of choices originally accepted in the CLI https://github.com/pytorch/serve/blob/master/model-archiver/model_archiver/arg_parser.py#L102. Should we add "tar" to the literal?

yes let's add

GeeCastro · 2023-10-31T23:19:34Z

model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py

-        convert=False,
        version=version,
-        source_vocab=source_vocab,


neither convert nor source_vocab are unsed in model archiver. Not here anyway, could this break a behaviour?

Let's keep them for now, this might be breaking something we're not testing

I think its okay to remove them. They are not part of the namespace created in argparse and only show up in this test. Presumably a relicts from before extra_files.

(serve) ubuntu@ip-172-31-55-226:~/serve$ grep -r convert model-archiver/ model-archiver/model_archiver/model_packaging_utils.py: logging.error("Failed to convert %s to the model-archive.", model_name) Binary file model-archiver/model_archiver/tests/unit_tests/__pycache__/test_model_packaging.cpython-310-pytest-7.3.1.pyc matches model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py: convert=False, model-archiver/build/lib/model_archiver/model_packaging_utils.py: logging.error("Failed to convert %s to the model-archive.", model_name) (serve) ubuntu@ip-172-31-55-226:~/serve$ grep -r source_vocab model-archiver/ Binary file model-archiver/model_archiver/tests/integ_tests/__pycache__/test_integration_model_archiver.cpython-310-pytest-7.3.1.pyc matches model-archiver/model_archiver/tests/integ_tests/test_integration_model_archiver.py: assert os.path.join(prefix, "source_vocab.pt") in file_list model-archiver/model_archiver/tests/integ_tests/default_handler_configuration.json: "extra-files": "model_archiver/tests/integ_tests/resources/regular_model/test_index_to_name.json,model_archiver/tests/integ_tests/resources/regular_model/source_vocab.pt", Binary file model-archiver/model_archiver/tests/unit_tests/__pycache__/test_model_packaging.cpython-310-pytest-7.3.1.pyc matches model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py: source_vocab = None model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py: source_vocab=source_vocab,

msaroufim · 2023-11-01T00:32:25Z

Just missing docs but nice to see CI green

mreso

Left some minor comments. LGTM otherwise

mreso · 2023-11-02T23:01:23Z

model-archiver/model_archiver/model_archiver_config.py

+    @staticmethod
+    def from_args(args: Namespace) -> "ModelArchiverConfig":
+        config = ModelArchiverConfig(
+            model_name=args.model_name,


The parameters and fields between args and config should always be aligned. Would be good to make the argument transfer automatic to remove one point that we need to touch in case we alter the parameters. Something like this:

from dataclasses import fields # ... @classmethod def from_args(cls, args: Namespace) -> "ModelArchiverConfig": params = {field.name: getattr(args, field.name) for field in fields(cls)} config = cls(**params) return config

mreso · 2023-11-02T23:12:20Z

model-archiver/model_archiver/tests/unit_tests/test_model_archiver.py

+        )
+        config = ModelArchiverConfig.from_args(args)
+
+        config == self.config


Would be good to ass assert here so the test fails if unequal

mreso · 2023-11-02T23:20:55Z

model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py

-        convert=False,
        version=version,
-        source_vocab=source_vocab,


I think its okay to remove them. They are not part of the namespace created in argparse and only show up in this test. Presumably a relicts from before extra_files.

(serve) ubuntu@ip-172-31-55-226:~/serve$ grep -r convert model-archiver/ model-archiver/model_archiver/model_packaging_utils.py: logging.error("Failed to convert %s to the model-archive.", model_name) Binary file model-archiver/model_archiver/tests/unit_tests/__pycache__/test_model_packaging.cpython-310-pytest-7.3.1.pyc matches model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py: convert=False, model-archiver/build/lib/model_archiver/model_packaging_utils.py: logging.error("Failed to convert %s to the model-archive.", model_name) (serve) ubuntu@ip-172-31-55-226:~/serve$ grep -r source_vocab model-archiver/ Binary file model-archiver/model_archiver/tests/integ_tests/__pycache__/test_integration_model_archiver.cpython-310-pytest-7.3.1.pyc matches model-archiver/model_archiver/tests/integ_tests/test_integration_model_archiver.py: assert os.path.join(prefix, "source_vocab.pt") in file_list model-archiver/model_archiver/tests/integ_tests/default_handler_configuration.json: "extra-files": "model_archiver/tests/integ_tests/resources/regular_model/test_index_to_name.json,model_archiver/tests/integ_tests/resources/regular_model/source_vocab.pt", Binary file model-archiver/model_archiver/tests/unit_tests/__pycache__/test_model_packaging.cpython-310-pytest-7.3.1.pyc matches model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py: source_vocab = None model-archiver/model_archiver/tests/unit_tests/test_model_packaging.py: source_vocab=source_vocab,

mreso

Quickly addressed the comments myself as I want to use this feature in master :-) ; thanks for contributing @Chichilele!

GeeCastro · 2023-11-04T06:50:13Z

Happy this is useful. I’m on holiday with limited access to a laptop so wouldn’t have been able to address this until next week.

mreso · 2023-11-04T06:58:09Z

No worries! Enjoy your holidays!

…

On Fri, Nov 3, 2023 at 23:50 G ***@***.***> wrote: Happy this is useful. I’m on holiday with limited access to a laptop so wouldn’t have been able to address this until next week. — Reply to this email directly, view it on GitHub <#2751 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADFYED747CBHQLZ54KHNATDYCXQTBAVCNFSM6AAAAAA6WZKBSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTGM3DGMBZHE> . You are receiving this because you commented.Message ID: ***@***.***>

GeeCastro added 6 commits October 30, 2023 21:58

add archiver config

322b821

add model archiver

dc44ed4

add to init

f79b4d4

integrated ModelArchiverConfig in existing scripts

b60b0a2

remove sys.exit mock

d45b3b1

add ModelArchiver unit test

182621d

fix tests

4db275c

msaroufim mentioned this pull request Oct 31, 2023

Kill install_dependencies.py (for conda) #2753

Open

GeeCastro added 2 commits October 31, 2023 22:31

ModelArchiverConfig to dataclass

2d4ac04

fix regression tests

3e3609c

GeeCastro commented Oct 31, 2023

View reviewed changes

msaroufim marked this pull request as ready for review November 1, 2023 00:20

mreso requested changes Nov 2, 2023

View reviewed changes

mreso added 2 commits November 3, 2023 18:26

Simplify args->config conversion; clean up test

27866ac

Merge branch 'master' into model-archiver-api

22eb9c8

mreso approved these changes Nov 3, 2023

View reviewed changes

msaroufim self-requested a review November 3, 2023 19:14

msaroufim approved these changes Nov 3, 2023

View reviewed changes

msaroufim enabled auto-merge November 3, 2023 19:15

msaroufim added this pull request to the merge queue Nov 3, 2023

Merged via the queue into pytorch:master with commit 3193748 Nov 3, 2023
13 checks passed

GeeCastro deleted the model-archiver-api branch November 4, 2023 06:50

chauhang added this to the v0.10.0 milestone Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model archiver api #2751

Model archiver api #2751

GeeCastro commented Oct 30, 2023

msaroufim commented Oct 30, 2023

codecov bot commented Oct 30, 2023 •

edited

Loading

GeeCastro Oct 31, 2023

msaroufim Nov 1, 2023

GeeCastro Oct 31, 2023

msaroufim Nov 1, 2023

mreso Nov 2, 2023

msaroufim commented Nov 1, 2023

mreso left a comment

mreso Nov 2, 2023

mreso Nov 2, 2023

mreso Nov 2, 2023

mreso left a comment

GeeCastro commented Nov 4, 2023

mreso commented Nov 4, 2023 via email

Model archiver api #2751

Model archiver api #2751

Conversation

GeeCastro commented Oct 30, 2023

Description

Type of change

Feature/Issue validation/testing

Checklist:

msaroufim commented Oct 30, 2023

codecov bot commented Oct 30, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msaroufim commented Nov 1, 2023

mreso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreso left a comment

Choose a reason for hiding this comment

GeeCastro commented Nov 4, 2023

mreso commented Nov 4, 2023 via email

codecov bot commented Oct 30, 2023 •

edited

Loading