feat: SDK code #3
Conversation
|
|
||
| match = "(main)" | ||
| upload_to_release = true | ||
| build_command = "pip install poetry && poetry build" |
There was a problem hiding this comment.
Yes. its for python-semantic-release
pyproject.toml
Outdated
| ] | ||
|
|
||
| match = "(main)" | ||
| upload_to_release = true |
There was a problem hiding this comment.
This is not used. Removed
| @@ -0,0 +1,4 @@ | |||
| import ai21 | |||
There was a problem hiding this comment.
Does this work? I think I'm missing what's up here
ai21/clients/studio/ai21_client.py
Outdated
| from ai21.tokenizers.factory import get_tokenizer | ||
|
|
||
|
|
||
| class AI21Client(AI21StudioClient): |
There was a problem hiding this comment.
Please rethink the structure of this class. IMO it should be simplified:
- I'm not sure why the client that's externally facing needs to extend the AI21StudioClient. IMO in the init you can create the client and then pass it to the resources. Let's try to avoid unnecessary inheritance when not needed
- I think that a pattern of passing
selfto your members can complicate things (such as recursive dependency tree) and should be avoided. This also correlates with the 1st suggestion here.
There was a problem hiding this comment.
I created the AI21StudioClient inside the constructor
ai21/clients/studio/ai21_client.py
Outdated
| self.library = StudioLibrary(self) | ||
| self.segmentation = StudioSegmentation(self) | ||
|
|
||
| def count_token(self, text: str, model_id: str = "j2-instruct") -> int: |
There was a problem hiding this comment.
you are not using the model_id here, so why expose it?
There was a problem hiding this comment.
Forgot to remove it. Done
| text: str, | ||
| *, | ||
| style: Optional[str] = None, | ||
| start_index: Optional[int] = 0, |
There was a problem hiding this comment.
What happens if start_index is passed as None?
There was a problem hiding this comment.
The server translates it to 0 by checking if the value is None
| count_penalty: Optional[Dict[str, Any]] = None, | ||
| **kwargs, | ||
| ) -> CompletionsResponse: | ||
| body = { |
There was a problem hiding this comment.
Isn't it a dup of Sagemaker resource?
There was a problem hiding this comment.
Bedrock receives model_id as well
ai21/resources/bedrock_resource.py
Outdated
| from typing import Any, Dict, TYPE_CHECKING | ||
|
|
||
| if TYPE_CHECKING: | ||
| from ai21.clients.bedrock.ai21_bedrock_client import AI21BedrockClient |
There was a problem hiding this comment.
I think there are to many conditional imports and it's going to be difficult to maintain.
Consider adding in the resource creation (or in __init__) a try+import+except before adding the resource to the client or exposing it in __init__ (Along with a proper error)
You can get insipration from Langchain and their conditional imports
There was a problem hiding this comment.
the error message for bad import would be
ImportError: Please install "ai21[AWS]" in order to use AI21BedrockClient
ai21/resources/bases/gec_base.py
Outdated
| return GECResponse.model_validate(json) | ||
|
|
||
| def _create_body(self, text: str, **kwargs) -> Dict[str, Any]: | ||
| return {"text": text, **kwargs} |
There was a problem hiding this comment.
Do we want to let users pass unrelated args?
There was a problem hiding this comment.
Come to think of it, not necessarily. It could allow for an indicative error, telling the user that this arg is no longer supported in case we decide to omit a field.
But for future proofing case - I'll remove it from the body param for now
ai21/utils.py
Outdated
| logger = logging.getLogger("ai21") | ||
|
|
||
|
|
||
| def log_info(message): |
There was a problem hiding this comment.
I only kept logger = logging.getLogger('ai21')
What we could also do is something like
def setup_logger():
if AI21EnvConfig.log_level == "debug":
logger.setLevel(logging.DEBUG)
elif AI21EnvConfig.log_level == "info":
logger.setLevel(logging.INFO)
...with maybe some more configs and then call it from our main __init__.py
No description provided.