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

Add fastai2_model_artifact #596

Merged
merged 15 commits into from
May 7, 2020
Merged

Add fastai2_model_artifact #596

merged 15 commits into from
May 7, 2020

Conversation

HenryDashwood
Copy link
Contributor

(Thanks for sending a pull request! Please make sure to read the contribution guidelines, then fill out the blanks below.)

What changes were proposed in this pull request?

Adds a fastai2_model_artifact

Does this close any currently open issues?

No

How was this patch tested?

@pep8speaks
Copy link

pep8speaks commented Apr 8, 2020

Hello @HenryDashwood, Thanks for updating this PR.

Line 34:89: E501 line too long (90 > 88 characters)
Line 47:89: E501 line too long (89 > 88 characters)
Line 95:1: W293 blank line contains whitespace

Line 106:1: W293 blank line contains whitespace

Comment last updated at 2020-05-05 22:13:41 UTC

@codecov-io
Copy link

codecov-io commented Apr 8, 2020

Codecov Report

Merging #596 into master will decrease coverage by 0.04%.
The diff coverage is 48.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   56.08%   56.04%   -0.05%     
==========================================
  Files          99      100       +1     
  Lines        7542     7585      +43     
==========================================
+ Hits         4230     4251      +21     
- Misses       3312     3334      +22     
Impacted Files Coverage Δ
bentoml/bundler/bundler.py 93.75% <ø> (ø)
bentoml/service.py 89.87% <ø> (ø)
bentoml/artifact/fastai2_model_artifact.py 47.61% <47.61%> (ø)
bentoml/artifact/__init__.py 100.00% <100.00%> (ø)

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 cacd9d7...8479245. Read the comment docs.

@parano parano self-requested a review April 21, 2020 00:30
@parano
Copy link
Member

parano commented May 5, 2020

@HenryDashwood thanks for updating the PR, I assume this is ready for review now? I will try it out on my end later today and update with you if I had any feedback.

@HenryDashwood
Copy link
Contributor Author

HenryDashwood commented May 5, 2020

Cheers! There's a slight difference for the user when saving the model as we have to use Sylvain's context manager:

with remove_patches_path():
    saved_path = service.save()

Would it be better if this were handled by Bentoml internally?

@parano
Copy link
Member

parano commented May 5, 2020

@HenryDashwood I'm planning to make the following change in BentoML to avoid that issue with fastcore's patch:

Instead of doing yaml.dump(y, Path("/tmp/test.yml")), we will use:

with open('/tmp/test.yaml', 'wb') as fp:
    yaml.dump(y, fp)

I will create a PR for that soon and test it together with your PR. I think after that change, the user won't need to use the remove_patches_path any more.

@HenryDashwood
Copy link
Contributor Author

Ah that's perfect then!

@parano parano merged commit 0f5eba9 into bentoml:master May 7, 2020
aarnphm pushed a commit to aarnphm/BentoML that referenced this pull request Jul 29, 2022
* added fastai2 artifact

* bring up to date with upstream master

* open pull request

* rebase

* delete write attr from Path and fix model path string in load

* rebase

* rebase

Co-authored-by: EC2 Default User <ec2-user@ip-172-16-79-16.eu-west-1.compute.internal>
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.

None yet

4 participants