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

Drop default api url for host of clients #209

Merged
merged 9 commits into from
Dec 5, 2024

Conversation

fgebhart
Copy link
Contributor

@fgebhart fgebhart commented Dec 4, 2024

No description provided.

@@ -217,7 +210,7 @@ def test_explanation_with_token_granularities_oom_regression(sync_client: Client

### Response:"""
target_text = " India won the Battle of Waterloo."
model_name = "luminous-base-control"
model_name = "luminous-base"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move away from control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it is (currently at least) not available in our new environment

@@ -18,7 +18,7 @@ Synchronous client.
from aleph_alpha_client import Client, CompletionRequest, Prompt
import os

client = Client(token=os.getenv("AA_TOKEN"))
client = Client(token=os.getenv("AA_TOKEN"), host="https://inference-api.your-domain.com")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not get the host from the .env, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. Probably the better approach, will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@martinreinhardt01 martinreinhardt01 left a comment

Choose a reason for hiding this comment

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

Easy to grasp MR and nice cleanup on the way.
Only two very minor questions.

@@ -24,7 +24,7 @@ jobs:
poetry run mypy tests --ignore-missing-imports
- name: Run tests
run: |
poetry run pytest
poetry run pytest --color=yes
Copy link
Contributor

Choose a reason for hiding this comment

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

Very important!

@fgebhart fgebhart merged commit 439634a into main Dec 5, 2024
6 checks passed
@fgebhart fgebhart deleted the drop-default-api-url-for-host-of-clients branch December 5, 2024 09:37
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.

3 participants