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

Add support for Azure authentication using Azure AD Tokens #165

Merged
merged 15 commits into from
Jul 3, 2024

Conversation

alaeddine-13
Copy link
Collaborator

@alaeddine-13 alaeddine-13 commented Jun 28, 2024

Description

Adds support for Azure authentication using Azure AD Tokens

Pull request checklist

  • Support from environment variable
  • Support from model YAML config
  • tests
    • Do we have tests? Should we add more?
    • Should we parametrize tests?
    • Should we add hypothesis tests?
  • documentation
    • add docstrings
    • if a module is new: add API reference file for Sphinx in docs/source/api-reference
    • check API doc of Sphinx - build and show with make sphinx && make open-sphinx
  • fix checks
    • start locale checks with make check
    • fix formatting with make format
  • Python type info
    • check and improve Python type info
    • double check if return types are specified - including -> None
  • check TODO comments in code
  • check FIXME comments in code
  • check if we want to increase the version or rc number
  • check if the copyright needs an update
    • file header
    • README.md
    • LICENSE file

@PhilipMay
Copy link
Member

@alaeddine-13 many thanks for thos PR.
Can you please add a copyright line to every file you touched.

Like this:

# Copyright (c) 2024 your_name_here, Deutsche Telekom AG

@PhilipMay
Copy link
Member

PhilipMay commented Jun 28, 2024

@alaeddine-13 please use the formatter. can do that with make format and make check.
That should fix some CI problems.

A very detailed guide is here: https://github.com/telekom/mltb2/blob/main/CONTRIBUTING.md#testing-linting-and-formatting

mltb2/openai.py Outdated Show resolved Hide resolved
mltb2/openai.py Outdated Show resolved Hide resolved
mltb2/openai.py Outdated Show resolved Hide resolved
mltb2/openai.py Outdated Show resolved Hide resolved
@PhilipMay
Copy link
Member

@alaeddine-13 please also add your copyright to the README.md and the LICENSE file.

@PhilipMay
Copy link
Member

Please increase the rc number in pyproject.toml.

@PhilipMay
Copy link
Member

Hi @alaeddine-13 ! To fix the CI issue with ruff: please do a rebase upstream/main. I fixed the issue there.

@alaeddine-13
Copy link
Collaborator Author

Hi @alaeddine-13 ! To fix the CI issue with ruff: please do a rebase upstream/main. I fixed the issue there.

done

@PhilipMay
Copy link
Member

We have this remaining mypy issue:

 Found 3 errors in 1 file (checked 28 source files)
mltb2/openai.py:369: note:          @classmethod
mltb2/openai.py:369: note:          def from_yaml(cls, yaml_file: Any, api_key: Optional[str] = ..., **kwargs: Any) -> Any
mltb2/openai.py:369: note:      Subclass:
mltb2/openai.py:369: note:          @classmethod
mltb2/openai.py:369: note:          def from_yaml(cls, yaml_file: Any, api_key: Optional[str] = ..., azure_ad_token: Optional[str] = ...) -> Any

Should we change back to "kwargs everywhere" or ignore this issue?

@PhilipMay
Copy link
Member

@alaeddine-13 mypy now fails with:

mltb2/openai.py:364: error: Argument "azure_endpoint" to "AsyncAzureOpenAI" has incompatible type "Optional[str]"; expected "str"  [arg-type]

The azure_endpoint is now optional in your PR. This is the reason for this issue. But AsyncAzureOpenAI expects it.
It is not optional there.

What is the reason to make azure_endpoint optional? Can that be changed back?

@alaeddine-13
Copy link
Collaborator Author

@PhilipMay can you run again the CI ? I implemented this workaround here in order to make azure_endpoint a positional argument

@alaeddine-13 alaeddine-13 requested a review from PhilipMay July 3, 2024 11:23
@PhilipMay PhilipMay merged commit 7c19235 into telekom:main Jul 3, 2024
8 checks passed
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