Skip to content

Conversation

@vbossica
Copy link
Contributor

No description provided.

@vbossica vbossica marked this pull request as ready for review June 24, 2025 13:40
@pescheck-bram
Copy link
Contributor

Hey @vbossica,

Thanks for adding Azure OpenAI support!

A few things to fix before merging:

  1. Refactor the model manager - Can you extract the Azure logic into a separate method instead of using # pylint: disable=too-many-branches? We prefer keeping the linting clean.

  2. Default model - The empty string for Azure OpenAI will break at runtime. Maybe use "gpt-35-turbo" as default?

  3. Tests - Could you add some basic tests for the Azure functionality?

The implementation looks good overall, just need these tweaks to maintain code quality. Feel free to ask if you have questions about any of these. And if you need help just ask, we are happy to help.

P.s. you can leave your name in the code if you want 👯

@vbossica
Copy link
Contributor Author

  1. Splitting the model manager into separate classes would be another option. You wouldn't have if/then anymore.
  2. It didn't break at runtime (just to say), but sure. It's moot anyway as models must first be installed, so there are no "list of models" of "default models" per so.
  3. Lemme see how to implement them.

@pescheck-bram
Copy link
Contributor

  1. Splitting the model manager into separate classes would be another option. You wouldn't have if/then anymore.
  2. It didn't break at runtime (just to say), but sure. It's moot anyway as models must first be installed, so there are no "list of models" of "default models" per so.
  3. Lemme see how to implement them.

Hey @vbossica,

Actually, if you're done with option 3 (the tests), we're fine with that and we can split the model manager afterwards. No need to hold up the PR for the refactoring.

Thanks for being so responsive to the feedback.

@vbossica
Copy link
Contributor Author

@pescheck-bram Done!

I had to remove some typing because Python 3.8 didn't like it. But since 3.8 is EOL, maybe removing it from the supported versions would make sense.

@pescheck-bram
Copy link
Contributor

Good one ticket created: #46

Merge request looks good to me!

@pescheck-bram pescheck-bram merged commit 8d8d189 into pescheckit:main Jun 24, 2025
17 checks passed
@vbossica vbossica deleted the azure_open_ai branch June 24, 2025 15:17
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.

2 participants