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

[Community]: Added classification_location parameter in PebbloSafeLoader. #22565

Merged

Conversation

rahul-trip
Copy link
Contributor

@rahul-trip rahul-trip commented Jun 5, 2024

Description: Add classifier_location feature flag. This flag enables Pebblo to decide the classifier location, local or pebblo-cloud.
Unit Tests: N/A
Documentation: N/A

Copy link

vercel bot commented Jun 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Jun 22, 2024 9:16pm

@rahul-trip rahul-trip force-pushed the rahul/pebblo/classification_location branch 7 times, most recently from e912a70 to 8ca58df Compare June 7, 2024 12:42
@rahul-trip rahul-trip force-pushed the rahul/pebblo/classification_location branch 2 times, most recently from ae09530 to 5544b59 Compare June 14, 2024 01:28
@rahul-trip rahul-trip force-pushed the rahul/pebblo/classification_location branch from 1b13f3a to 7729af6 Compare June 17, 2024 17:42
@rahul-trip rahul-trip marked this pull request as ready for review June 17, 2024 18:38
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 🤖:improvement Medium size change to existing code to handle new use-cases labels Jun 17, 2024
@rahul-trip
Copy link
Contributor Author

Hi @baskaryan
Kindly review. Thanks!

@ccurme ccurme added the community Related to langchain-community label Jun 18, 2024
@rahul-trip rahul-trip changed the title Added classification_location parameter in PebbloSafeLoader. [Community]: Added classification_location parameter in PebbloSafeLoader. Jun 19, 2024
@rahul-trip
Copy link
Contributor Author

Hi @eyurtsev, @hwchase17
Kindly review and merge if looks fine.

@eyurtsev eyurtsev self-assigned this Jun 20, 2024
@eyurtsev
Copy link
Collaborator

@Raj725 are you able to review?

@eyurtsev
Copy link
Collaborator

@rahul-trip if you're working at daxa-ai, merge time can be significantly reduced by getting another daxa-ai engineer to review the changes

@@ -321,38 +333,44 @@ def _get_app_details(app_name, owner, description, llm, **kwargs) -> App: # typ
return app

@staticmethod
def _send_discover(app, api_key, classifier_url) -> None: # type: ignore
def _send_discover(app, api_key, classifier_url, classifier_location) -> None: # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

type annotations missing here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added missing type annotations throughout file.

@@ -150,6 +150,7 @@ class Doc(BaseModel):
loader_details: dict
loading_end: bool
source_owner: str
classifier_location: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Could you update the doc-string?
  • The doc-strings should be below the attribute not in the model doc-string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all the models with suggested doc-string style.

@@ -46,6 +46,7 @@ def __init__(
api_key: Optional[str] = None,
load_semantic: bool = False,
classifier_url: Optional[str] = None,
classifier_location: str = "local",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use * prior to new named arguments

Copy link
Contributor Author

@rahul-trip rahul-trip Jun 22, 2024

Choose a reason for hiding this comment

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

Added as suggested.

@rahul-trip rahul-trip force-pushed the rahul/pebblo/classification_location branch 3 times, most recently from 7919297 to 1903c15 Compare June 22, 2024 21:02
@rahul-trip
Copy link
Contributor Author

rahul-trip commented Jun 22, 2024

@rahul-trip if you're working at daxa-ai, merge time can be significantly reduced by getting another daxa-ai engineer to review the changes

@eyurtsev we do review each PR internally and post the branch here only when internal review is approved (we do this in our fork.). From now on, we will start the approval process here as suggested.

@Raj725, please check if you are able to approve.

Signed-off-by: Rahul Tripathi <[email protected]>
@rahul-trip rahul-trip force-pushed the rahul/pebblo/classification_location branch from 1903c15 to 50c835b Compare June 22, 2024 21:16
@rahul-trip rahul-trip requested a review from eyurtsev June 23, 2024 18:48
@Raj725
Copy link
Contributor

Raj725 commented Jun 24, 2024

@Raj725 are you able to review?

@eyurtsev Apologies for the delay. I've reviewed it, and it looks good to me.

Copy link
Contributor

@Raj725 Raj725 left a comment

Choose a reason for hiding this comment

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

LGTM

@eyurtsev eyurtsev merged commit 9ef93ec into langchain-ai:master Jun 24, 2024
51 checks passed
@eyurtsev
Copy link
Collaborator

@rahul-trip / @Raj725 thanks! Your existing process is great. Mainly helpful to see a final sign off from another person on the team!

Josephasafg pushed a commit to joshc-ai21/langchain that referenced this pull request Jun 25, 2024
…feLoader. (langchain-ai#22565)

Description: Add classifier_location feature flag. This flag enables
Pebblo to decide the classifier location, local or pebblo-cloud.
Unit Tests: N/A
Documentation: N/A

---------

Signed-off-by: Rahul Tripathi <[email protected]>
Co-authored-by: Rahul Tripathi <[email protected]>
mandos21 pushed a commit to mandos21/langchain that referenced this pull request Jul 8, 2024
…feLoader. (langchain-ai#22565)

Description: Add classifier_location feature flag. This flag enables
Pebblo to decide the classifier location, local or pebblo-cloud.
Unit Tests: N/A
Documentation: N/A

---------

Signed-off-by: Rahul Tripathi <[email protected]>
Co-authored-by: Rahul Tripathi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Related to langchain-community 🤖:improvement Medium size change to existing code to handle new use-cases size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants