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.

Sign up for GitHub

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

Jump to bottom

Refine KerasModelArtifact & its integration test #1295

Merged
merged 8 commits into from
Dec 4, 2020
Merged

Refine KerasModelArtifact & its integration test #1295

merged 8 commits into from
Dec 4, 2020

Conversation

bojiang
Copy link
Member

@bojiang bojiang commented Dec 3, 2020 •
edited

Description

Motivation and Context

See linked issues.

How Has This Been Tested?

Added Integration tests for:

  • standalone keras with tensorflow 1.x
  • tf.keras with tensorflow 1.14
  • standalone keras with tensorflow 2.x
  • tf.keras with tensorflow 2.x

both with store_as_json_and_weights=True and false.

Also tested on gallery/legacy-keras examples.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature and improvements (non-breaking change which adds/improves functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Code Refactoring (internal change which is not user facing)
  • Documentation
  • Test, CI, or build

Component(s) if applicable

  • BentoService (service definition, dependency management, API input/output adapters)
  • Model Artifact (model serialization, multi-framework support)
  • Model Server (mico-batching, dockerisation, logging, OpenAPI, instruments)
  • YataiService gRPC server (model registry, cloud deployment automation)
  • YataiService web server (nodejs HTTP server and web UI)
  • Internal (BentoML's own configuration, logging, utility, exception handling)
  • BentoML CLI

Checklist:

  • My code follows the bentoml code style, both ./dev/format.sh and
    ./dev/lint.sh script have passed
    (instructions).
  • My change reduces project test coverage and requires unit tests to be added
  • I have added unit tests covering my code change
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@codecov
Copy link

codecov bot commented Dec 3, 2020 •
edited

Codecov Report

Merging #1295 (48f5b2a) into master (9a5b6cb) will increase coverage by 0.92%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1295      +/-   ##
==========================================
+ Coverage   66.25%   67.18%   +0.92%     
==========================================
  Files         144      144              
  Lines        9332     9313      -19     
==========================================
+ Hits         6183     6257      +74     
+ Misses       3149     3056      -93
Impacted Files Coverage Δ
bentoml/frameworks/keras.py 69.15% <42.85%> (+69.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a5b6cb...48f5b2a. Read the comment docs.

This was linked to issues Dec 3, 2020
KerasModelArtifact: local variable 'keras_module' referenced before assignment #1185
Closed
Add integration tests for KerasModelArtifact #1196
Closed
Usage of model._make_predict_function() makes it impossible to use KerasModelArtifact with tensorflow 2 #1200
Closed
@bojiang bojiang requested a review from parano December 3, 2020 15:46
@bojiang bojiang linked an issue Dec 3, 2020 that may be closed by this pull request
Better TensorFlow integration #1188
Closed
3 tasks
@bojiang bojiang mentioned this pull request Dec 3, 2020
Closed
18 tasks
parano
parano reviewed Dec 3, 2020
View reviewed changes
bentoml/frameworks/keras.py
@@ -191,8 +187,11 @@ def load(self, path):
"Failed to import '{}' module when loading saved "
"KerasModelArtifact".format(keras_module_name)
)

self.create_session()
else:
Copy link
Member

Choose a reason for hiding this comment

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

side note: the else branch was left unhandled for back-ward compatibility reason when this was first created. But I think it is fine raising this exception here now since this was introduced over a year ago.

parano
parano reviewed Dec 3, 2020
View reviewed changes
tests/bento_service_examples/keras_classifier.py
@bentoml.artifacts(
[
KerasModelArtifact('model'),
KerasModelArtifact('model2', store_as_json_and_weights=True),
Copy link
Member

Choose a reason for hiding this comment

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

👍

parano
parano reviewed Dec 3, 2020
View reviewed changes
.github/workflows/ci.yml
- name: Run tests with tf2
run: ./ci/keras_integration_tests.sh
- name: Run tests with tf1
run: ./ci/keras_with_tf1_integration_tests.sh
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to put them into two separate tasks?

@parano
Copy link
Member

parano commented Dec 3, 2020

LGTM, great job adding the tests @bojiang!

Looks like the yatai service integration tests are still a bit flaky cc @yubozhao

@yubozhao
Copy link
Contributor

yubozhao commented Dec 4, 2020

Fixing the flaky yatai service test. If it continues to cause an issue, I will skip the test(one of the two tests for yatai service) that caused it

@yubozhao yubozhao merged commit d00be30 into bentoml:master Dec 4, 2020
@bojiang bojiang linked an issue Dec 4, 2020 that may be closed by this pull request
[TEST/CI] Add tests covering KerasModelArtifact #722
Closed
uhbuhb pushed a commit to MLH-Fellowship/BentoML that referenced this pull request Feb 17, 2021
@bojiang @uhbuhb
Refine KerasModelArtifact & its integration test (bentoml#1295)
73596f5 
* [CI] integration tests for keras

* [FIX] keras artifact

* fix for tf1

* test for json n weights

* add script for keras with tf1

* add to github action
aarnphm pushed a commit to aarnphm/BentoML that referenced this pull request Jul 29, 2022
@bojiang
Refine KerasModelArtifact & its integration test (bentoml#1295)
5e8b79a 
* [CI] integration tests for keras

* [FIX] keras artifact

* fix for tf1

* test for json n weights

* add script for keras with tf1

* add to github action
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parano parano parano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@bojiang @parano @yubozhao