Skip to content

Conversation

mq200
Copy link

@mq200 mq200 commented Sep 6, 2024

Add IBM Watsonx.ai client for use with CodeGPT Watsonx.ai integration

@mq200 mq200 changed the title Watsonx client feat: Watsonx client Sep 6, 2024
@mq200 mq200 marked this pull request as draft September 6, 2024 19:19
@carlrobertoh
Copy link
Owner

carlrobertoh commented Sep 10, 2024

There's a codestyle rules in config/codestyle.xml. Could you please apply it?

}
}

public EventSource getCompletionAsync(
Copy link
Owner

Choose a reason for hiding this comment

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

Can we write some integration tests for these streaming methods? It will make refactoring easier later on. You can use other int. tests as an example.

@mq200 mq200 force-pushed the master branch 3 times, most recently from 9c90c40 to 3bf1fe7 Compare September 14, 2024 20:27

public class WatsonxAuthenticator {

IBMAuthBearerToken bearerToken;
Copy link
Owner

Choose a reason for hiding this comment

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

These fields could be private if they're not accessed outside this class

.addHeader("Content-Type", "application/x-www-form-urlencoded")
.build();
try {
Response response = client.newCall(request).execute();
Copy link
Owner

Choose a reason for hiding this comment

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

I don't consider creating new bearer tokens on object initiation a good practise, please reconsider using a lazy initialization

}

// Watsonx API Key
public WatsonxAuthenticator(String username, String apiKey,
Copy link
Owner

Choose a reason for hiding this comment

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

The constructor is too complex. Can we break it down into smaller units for better readability?


private void generateNewBearerToken() {
try {
Response response = client.newCall(request).execute();
Copy link
Owner

Choose a reason for hiding this comment

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

Resources need to be cleaned up - try (Response response = client.newCall(request).execute())

.getExpiry());
}
} catch (IOException e) {
System.out.println(e);
Copy link
Owner

Choose a reason for hiding this comment

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

Please re-throw the exception with meaningful message


public String getBearerTokenValue() {
if (!isZenApiKey && (this.bearerToken == null || (this.bearerToken.getExpiration() * 1000)
< (new Date().getTime() + 60000))) {
Copy link
Owner

Choose a reason for hiding this comment

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

The magic number 60000 could be named constant for clarity

.getMessage();
return message == null ? "" : message;
} catch (Exception ex) {
System.out.println(ex);
Copy link
Owner

Choose a reason for hiding this comment

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

Please use proper logging

WatsonxCompletionParameters parameters;

public WatsonxCompletionRequest(Builder builder) {
System.out.println("Model ID: " + builder.modelId);
Copy link
Owner

Choose a reason for hiding this comment

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

Can we clean this up?

@carlrobertoh carlrobertoh force-pushed the master branch 2 times, most recently from 0cb1e40 to 8a2bf0a Compare July 21, 2025 10:02
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