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

Gpt2 notebook example #3217

Merged
merged 4 commits into from
May 21, 2021
Merged

Conversation

nadinet
Copy link
Contributor

@nadinet nadinet commented May 21, 2021

What this PR does / why we need it:
Extend notebook to contain an example of a simple load test using Vegeta
Which issue(s) this PR fixes:

Fixes #3216

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -9,17 +9,20 @@
"\n",
Copy link
Contributor

@adriangonz adriangonz May 21, 2021

Choose a reason for hiding this comment

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

Nice one on adding the default namespace explicitly


Reply via ReviewNB

@@ -9,17 +9,20 @@
"\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the try / except block? I.e. just leave the res = ret.json() to simplify the snippet?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this block as when running in loop (while) may cause some request to return with empty ret => ret.json() throw exception. I believe it is a timing issue/ load on the model as without calling request in loop never happens to me. I added the try block to make sure the script returns a sentences completion.

I find it O.K to leave it that way - I don't think it impairs the readability / understanding. Let me know if you prefer it to be removed

@@ -9,17 +9,20 @@
"\n",
Copy link
Contributor

@adriangonz adriangonz May 21, 2021

Choose a reason for hiding this comment

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

Would it make sense to run black over this snippet and the previous one?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense - ran it

@axsaucedo
Copy link
Contributor

/approve

@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: axsaucedo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@seldondev seldondev merged commit 9ce5dfd into SeldonIO:master May 21, 2021
@seldondev
Copy link
Collaborator

Failed to merge this PR due to:

failed merging [3217]: [Method Not Allowed]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GPT2-Triton Example: extand to contain load test example
4 participants