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

Update docs of DeepsetCloudDocumentStore #2460

Merged
merged 5 commits into from
Apr 27, 2022
Merged

Update docs of DeepsetCloudDocumentStore #2460

merged 5 commits into from
Apr 27, 2022

Conversation

tholor
Copy link
Member

@tholor tholor commented Apr 26, 2022

Proposed changes:
Updating a few doc strings of the DeepsetCloudDocumentStore

@tstadel not sure about the usage of "DEFAULT_API_ENDPOINT". Where is this one actually used? Does it make sense to update it to the actual endpoint now?

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@tholor tholor requested a review from tstadel April 26, 2022 14:38
Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

DEFAULT_API_ENDPOINT in haystack/document_stores/deepsetcloud.py can be safely removed. Instead please adjust DEFAULT_API_ENDPOINT in haystack/utils/deepsetcloud.py. This is the one that acts as default if neither the api_key param nor the env variable has been set.
The remaining parts look good to me :-)

@@ -9,7 +9,7 @@
from haystack.utils import DeepsetCloud


DEFAULT_API_ENDPOINT = f"DC_API_PLACEHOLDER/v1" # TODO
DEFAULT_API_ENDPOINT = "https://api.cloud.deepset.ai/api/v1"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is superfluous. The actual DEFAULT_API_ENDPOINT has moved to haystack/utils/deepsetcloud.py. There it makes sense to adjust it. But this one can be safely removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, adjusted it

haystack/document_stores/deepsetcloud.py Outdated Show resolved Hide resolved
tholor and others added 3 commits April 26, 2022 16:52
Co-authored-by: tstadel <60758086+tstadel@users.noreply.github.com>
@tholor tholor requested a review from tstadel April 26, 2022 15:12
Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

LGTM!

@tstadel tstadel merged commit 766e753 into master Apr 27, 2022
@tstadel tstadel deleted the tholor-patch-1 branch April 27, 2022 17:40
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.

None yet

3 participants