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

feat: add nebullvm as backend #697

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

diegofiori
Copy link

Add nebullvm as a model backend in a similar way to ONNXruntime backend

@numb3r3
Copy link
Member

numb3r3 commented Apr 27, 2022

awsome job. The current PR cannot pass CI checking. Please follow this guidelines https://github.com/jina-ai/jina/blob/master/CONTRIBUTING.md

@numb3r3
Copy link
Member

numb3r3 commented Apr 27, 2022

BTW., please also use pre-commit to format your codes

$ pre-commit install
$ git commit -am "feat: nebullm backend"

Copy link
Member

@numb3r3 numb3r3 left a comment

Choose a reason for hiding this comment

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

please check my comments 🍻

@hanxiao
Copy link
Member

hanxiao commented Apr 27, 2022

hi @morgoth95 is it possible to open PR modification permission to us? we could take care of it on the styling and minors things. Reference here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@diegofiori
Copy link
Author

hi @morgoth95 is it possible to open PR modification permission to us? we could take care of it on the styling and minors things. Reference here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Hello @hanxiao! Actually, I already activated the option "Allow edits by maintainers" when I created the PR 😄 In theory all of you with writing access to clip-as-a-service should be able to directly push new commits to my branch 😉

README.md Outdated
@@ -45,13 +45,20 @@ curl -X POST http://demo-cas.jina.ai:51001/post -H 'Content-Type: application/js
## Install

CLIP-as-service consists of two Python packages `clip-server` and `clip-client` that can be installed _independently_. Both require Python 3.7+.
`nebullvm` installation will need on some platforms python
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is more about the setup. It's better to be placed in the following Install server section.

Copy link
Author

Choose a reason for hiding this comment

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

fixed in 01f7f08

server/clip_server/executors/clip_nebullvm.py Show resolved Hide resolved
server/clip_server/model/clip_nebullvm.py Outdated Show resolved Hide resolved
server/clip_server/model/clip_nebullvm.py Outdated Show resolved Hide resolved
Copy link
Member

@numb3r3 numb3r3 left a comment

Choose a reason for hiding this comment

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

And please add unit test for nebullvm

@diegofiori diegofiori requested a review from numb3r3 May 3, 2022 08:00

python -m clip_server nebullvm_flow.yml
Copy link
Member

Choose a reason for hiding this comment

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

should be nebullvm-flow.yml

Copy link
Author

Choose a reason for hiding this comment

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

solved in f06133e


def __enter__(self):
if self.device == "cpu" and torch.cuda.is_available():
self.cuda_str = os.environ.get("CUDA_VISIBLE_DEVICES") or "1"
Copy link
Member

Choose a reason for hiding this comment

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

Is this a bug? maybe we can use self.cuda_str = os.environ.get('CUDA_VISIBLE_DEVICES', None), otherwise the CUDA_VISIBLE_DEVICES will be updated to "1" at exiting.

Copy link
Author

Choose a reason for hiding this comment

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

Solved in f9ce0ae

@numb3r3
Copy link
Member

numb3r3 commented May 9, 2022

@morgoth95 how is going now? Just wondering do we have the chance to merge this PR this week?

@diegofiori
Copy link
Author

@morgoth95 how is going now? Just wondering do we have the chance to merge this PR this week?

Sorry for the delay, we were working on the latest release of nebullvm! I should have fixed the issues you pointed out! I think we can try to accelerate the process and merge the PR this week.

@diegofiori diegofiori requested a review from numb3r3 May 10, 2022 09:55
@numb3r3
Copy link
Member

numb3r3 commented May 11, 2022

@morgoth95 Congrats. nebullvm v0.3.0 comes yesterday, very cool. And there are still some bad commit messages in this PR, to fix them please refer to jina-ai/serve#553

@diegofiori diegofiori force-pushed the main branch 2 times, most recently from c0808e0 to b42d031 Compare May 11, 2022 16:33
@numb3r3
Copy link
Member

numb3r3 commented May 12, 2022

FYI, this PR has one ore more bad commit messages,


⧗   input: docs: Fix typo in docs
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]

@diegofiori
Copy link
Author

@numb3r3

FYI, this PR has one ore more bad commit messages,


⧗   input: docs: Fix typo in docs
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]

it should have been fixed now! Let me know if further changes are needed

@numb3r3
Copy link
Member

numb3r3 commented May 16, 2022

@morgoth95 Please rebase this PR so that it does not show unwanted commits

@numb3r3
Copy link
Member

numb3r3 commented May 17, 2022

@morgoth95 please remember to install pre-commit to format your code before commits:

$ pre-commit install
$ git commit -am "fix: xxxx"

Copy link
Member

@numb3r3 numb3r3 left a comment

Choose a reason for hiding this comment

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

please format your code via:

$ pre-commit run --all

@diegofiori
Copy link
Author

please format your code via:

$ pre-commit run --all

Solved in 932cfe6

@diegofiori diegofiori requested a review from numb3r3 May 18, 2022 06:57
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #697 (932cfe6) into main (87fdc54) will decrease coverage by 33.71%.
The diff coverage is 0.00%.

❗ Current head 932cfe6 differs from pull request most recent head dce8c09. Consider uploading reports for the commit dce8c09 to get more accurate results

@@             Coverage Diff             @@
##             main     #697       +/-   ##
===========================================
- Coverage   81.58%   47.87%   -33.72%     
===========================================
  Files          21       18        -3     
  Lines        1575     1201      -374     
===========================================
- Hits         1285      575      -710     
- Misses        290      626      +336     
Flag Coverage Δ
cas 47.87% <0.00%> (-33.72%) ⬇️

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

Impacted Files Coverage Δ
server/clip_server/executors/clip_nebullvm.py 0.00% <0.00%> (ø)
server/clip_server/model/clip_nebullvm.py 0.00% <0.00%> (ø)
server/clip_server/executors/clip_torch.py 0.00% <0.00%> (-83.34%) ⬇️
server/clip_server/executors/clip_onnx.py 0.00% <0.00%> (-81.95%) ⬇️
server/clip_server/model/model.py 15.83% <0.00%> (-54.03%) ⬇️
server/clip_server/model/clip.py 47.85% <0.00%> (-39.65%) ⬇️
client/clip_client/client.py 64.83% <0.00%> (-23.95%) ⬇️
server/clip_server/model/clip_onnx.py 52.00% <0.00%> (-20.73%) ⬇️
server/clip_server/executors/helper.py 91.22% <0.00%> (-5.84%) ⬇️
server/clip_server/model/simple_tokenizer.py 93.87% <0.00%> (-1.03%) ⬇️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@numb3r3
Copy link
Member

numb3r3 commented May 20, 2022

@morgoth95 The unittest seems broken

@numb3r3
Copy link
Member

numb3r3 commented Jun 1, 2022

@morgoth95 Hi, I hope you are well. Did you get a chance to continue this PR to pass the unit test?

@diegofiori
Copy link
Author

@morgoth95 Hi, I hope you are well. Did you get a chance to continue this PR to pass the unit test?

Hi Felix, I'm working on the tests. A couple of days and I'll close the open tasks. Looking forward to the integration!

@diegofiori
Copy link
Author

@numb3r3 Hi, I hope all is fine on the Jina side. I took a look at the failed tests and I may have identified the source of the problem. Unfortunately it will require some work on our end, but we think it will be fixed with the next nebullvm release (it will happen in about a week ;)). I will get back to you as soon as the problem is fixed :)

@numb3r3
Copy link
Member

numb3r3 commented Jun 30, 2022

@morgoth95 very nice, looking forward the next nebullvm release.

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.

4 participants