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

DEID-2609 Added NERTextResponse class and changes to support the NER … #41

Merged
merged 9 commits into from
Jul 22, 2024
Merged

DEID-2609 Added NERTextResponse class and changes to support the NER … #41

merged 9 commits into from
Jul 22, 2024

Conversation

AmirPAI
Copy link
Contributor

@AmirPAI AmirPAI commented Jun 13, 2024 •
edited
Loading

…route

What's Changed?

  • Updated the PAIPostRequests object to contain the new NER route
  • Created a NERTextResponse
  • Create the NERTextRequest
  • Added Unit tests for NER Text
  • Added Integration tests for NER Text
  • Removed outdated default_link_batch parameter.

PR Checklist

  • Updated unit tests
  • Ran unit tests
  • Ran Integration tests

@AmirPAI AmirPAI requested review from a-guiducci and guyd June 13, 2024 10:06
a-guiducci
a-guiducci previously requested changes Jun 13, 2024
View reviewed changes
Copy link
Collaborator

@a-guiducci a-guiducci left a comment

Choose a reason for hiding this comment

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

Based on my first comment it appears to me that you haven't tested these changes on a container. IMO you should probably make sure it works with integration testing before opening a pull request.

Could you also add integration tests to this change please 🙏

src/privateai_client/components/pai_requests.py Outdated Show resolved Hide resolved
@a-guiducci a-guiducci dismissed their stale review June 13, 2024 13:20

Sorry, didn't notice this was still in draft!

@a-guiducci a-guiducci self-requested a review June 13, 2024 13:20
@AmirPAI AmirPAI marked this pull request as ready for review June 18, 2024 14:09
a-guiducci
a-guiducci approved these changes Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@a-guiducci a-guiducci left a comment

Choose a reason for hiding this comment

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

Can you also upload the changeme with a summary of your additions?

Have a couple small comments, otherwise looks good! Are these features in prod yet? If not let's hold off merging until they're released

src/privateai_client/components/pai_requests.py
def ner_text(self, request_object):
return self.make_request(
self.request_type, self.uris.ner_text, request_object
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a change request in pai_requests.py? It's not appearing.

src/privateai_client/components/request_objects.py Outdated
@@ -759,6 +759,37 @@ def fromdict(cls, values: dict):
)


class NerTextRequest(BaseRequestObject):
default_link_batch = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this being used. Do you want the initializer to default link_batch to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @a-guiducci , Thanks for the approval.
The NerText has most of its properties similar to ProcessText, so I have set the initializer to the same default values.
@guyd can you verify this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see what you're saying. Ok np. I think the default value for process text was removed at some point but never really cleaned up :/

If you don't mind, let's remove the default_link_batch from the NerTextRequest because we'll never use it 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok. Sure

@guyd
Copy link
Contributor

guyd commented Jul 2, 2024

@AmirPAI is this PR ready to be merged? Do you need help to complete any missing pieces?

@AmirPAI
Copy link
Contributor Author

AmirPAI commented Jul 3, 2024

Yes @guyd checks are done and this PR is ready to be merged. As Adam suggested, I'm holding the merge until DEID-NER branch gets merged first (which is in-turn waiting for a review from by Kory or Bryan).

yaysummeriscoming
yaysummeriscoming approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@yaysummeriscoming yaysummeriscoming left a comment

Choose a reason for hiding this comment

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

Didn't take a detailed look, but seems good to me. Thanks @AmirPAI

Merge branch 'main' of https://github.com/privateai/pai-thin-client i…
3ac2625 
…nto DEID-2609-update-the-python-client-with-the-ner-route
@AmirPAI
Copy link
Contributor Author

AmirPAI commented Jul 22, 2024

Now that NER route has been merged into master branch, I've tested it again after spinning up the container on master branch and done the tests. All tests have passed successfully. Merging!

@AmirPAI AmirPAI merged commit 4117e5f into main Jul 22, 2024
@a-guiducci
Copy link
Collaborator

@AmirPAI you never addressed my CHANGELOG comment. Please open a new pr with those changes and you have now provided false information for what's in our 3.8.2 version of the client

@a-guiducci
Copy link
Collaborator

Disregard. I'm just updating it myself

@AmirPAI
Copy link
Contributor Author

AmirPAI commented Jul 26, 2024

Hey @a-guiducci , My bad, mistakenly put those changelog summary lines in under 3.8.2 instead of 3.9.0.
Thanks for that.

@bryanbellsmith bryanbellsmith deleted the DEID-2609-update-the-python-client-with-the-ner-route branch October 18, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-guiducci a-guiducci a-guiducci approved these changes

@yaysummeriscoming yaysummeriscoming yaysummeriscoming approved these changes

@guyd guyd Awaiting requested review from guyd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AmirPAI @guyd @a-guiducci @yaysummeriscoming