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.

Sign up for GitHub

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

[OPIK-735] Online Scoring: null projectId error when creating a rule #1011

Merged
merged 6 commits into from
Jan 10, 2025
Merged

[OPIK-735] Online Scoring: null projectId error when creating a rule #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
190e236
bugfix: projectId should be used from path parameter, not the one fro…
ldaugusto Jan 9, 2025
4217f50
fixing projectId view
ldaugusto Jan 9, 2025
f0e3876
fixing projectId view
ldaugusto Jan 9, 2025
8fc21bf
Merge branch 'main' into daniel/bugfix-evaluators-endpoints
ldaugusto Jan 10, 2025
eef2514
disabling chat completion tests until they're fixed
ldaugusto Jan 10, 2025
2d20b26
Merge branch 'main' into daniel/bugfix-evaluators-endpoints
ldaugusto Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apps/opik-backend/src/main/java/com/comet/opik/api/AutomationRuleEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import io.swagger.v3.oas.annotations.media.DiscriminatorMapping;
import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
Expand Down Expand Up @@ -35,8 +34,9 @@ public abstract sealed class AutomationRuleEvaluator<T>
@Schema(accessMode = Schema.AccessMode.READ_ONLY)
UUID id;

@JsonView({View.Public.class, View.Write.class})
@NotNull UUID projectId;
@JsonView({View.Public.class})
@Schema(accessMode = Schema.AccessMode.READ_ONLY)
UUID projectId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class is marked with @Data so it has setters and getters. However, all these fields should be marked as private.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that this is meant to be extendable, so ok to have default visibility or protected for extension outside the package. You can still opt to narrow down visibility to private if you prefer.


@JsonView({View.Public.class, View.Write.class})
@Schema(accessMode = Schema.AccessMode.READ_WRITE)
Expand Down
3 changes: 2 additions & 1 deletion .../src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public Response getEvaluator(@PathParam("projectId") UUID projectId, @PathParam(
})
@RateLimited
public Response createEvaluator(
@PathParam("projectId") UUID projectId,
@RequestBody(content = @Content(schema = @Schema(implementation = AutomationRuleEvaluator.class))) @JsonView(AutomationRuleEvaluator.View.Write.class) @NotNull @Valid AutomationRuleEvaluator<?> evaluator,
@Context UriInfo uriInfo) {

Expand All @@ -108,7 +109,7 @@ public Response createEvaluator(

log.info("Creating {} evaluator for project_id '{}' on workspace_id '{}'", evaluator.type(),
evaluator.getProjectId(), workspaceId);
AutomationRuleEvaluator<?> savedEvaluator = service.save(evaluator, workspaceId, userName);
AutomationRuleEvaluator<?> savedEvaluator = service.save(evaluator, projectId, workspaceId, userName);
log.info("Created {} evaluator '{}' for project_id '{}' on workspace_id '{}'", evaluator.type(),
savedEvaluator.getId(), evaluator.getProjectId(), workspaceId);

Expand Down
7 changes: 4 additions & 3 deletions apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
@ImplementedBy(AutomationRuleEvaluatorServiceImpl.class)
public interface AutomationRuleEvaluatorService {

<E, T extends AutomationRuleEvaluator<E>> T save(T automationRuleEvaluator, @NonNull String workspaceId,
@NonNull String userName);
<E, T extends AutomationRuleEvaluator<E>> T save(T automationRuleEvaluator, @NonNull UUID projectId,
@NonNull String workspaceId, @NonNull String userName);

void update(@NonNull UUID id, @NonNull UUID projectId, @NonNull String workspaceId, @NonNull String userName,
AutomationRuleEvaluatorUpdate automationRuleEvaluator);
Expand All @@ -58,10 +58,10 @@ class AutomationRuleEvaluatorServiceImpl implements AutomationRuleEvaluatorServi

private final @NonNull IdGenerator idGenerator;
private final @NonNull TransactionTemplate template;
private final int DEFAULT_PAGE_LIMIT = 10;

@Override
public <E, T extends AutomationRuleEvaluator<E>> T save(T inputRuleEvaluator,
@NonNull UUID projectId,
@NonNull String workspaceId,
@NonNull String userName) {

Expand All @@ -75,6 +75,7 @@ public <E, T extends AutomationRuleEvaluator<E>> T save(T inputRuleEvaluator,
case AutomationRuleEvaluatorLlmAsJudge llmAsJudge -> {
var definition = llmAsJudge.toBuilder()
.id(id)
.projectId(projectId)
.createdBy(userName)
.lastUpdatedBy(userName)
.build();
Expand Down
10 changes: 6 additions & 4 deletions ...a/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@ public class AutomationRuleEvaluatorResourceClient {
private final ClientSupport client;
private final String baseURI;

public UUID createEvaluator(AutomationRuleEvaluator<?> evaluator, String workspaceName, String apiKey) {
try (var actualResponse = createEvaluator(evaluator, workspaceName, apiKey, HttpStatus.SC_CREATED)) {
public UUID createEvaluator(AutomationRuleEvaluator<?> evaluator, UUID projectId, String workspaceName,
String apiKey) {
try (var actualResponse = createEvaluator(evaluator, projectId, workspaceName, apiKey, HttpStatus.SC_CREATED)) {
assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(201);

return TestUtils.getIdFromLocation(actualResponse.getLocation());
}
}

public Response createEvaluator(AutomationRuleEvaluator<?> evaluator, String workspaceName, String apiKey,
public Response createEvaluator(AutomationRuleEvaluator<?> evaluator, UUID projectId, String workspaceName,
String apiKey,
int expectedStatus) {
var actualResponse = client.target(RESOURCE_PATH.formatted(baseURI, evaluator.getProjectId()))
var actualResponse = client.target(RESOURCE_PATH.formatted(baseURI, projectId))
.request()
.accept(MediaType.APPLICATION_JSON_TYPE)
.header(HttpHeaders.AUTHORIZATION, apiKey)
Expand Down
14 changes: 7 additions & 7 deletions .../src/test/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListenerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class OnlineScoringEventListenerTest {
private static final String API_KEY = UUID.randomUUID().toString();
private static final String USER = UUID.randomUUID().toString();
private static final String WORKSPACE_ID = UUID.randomUUID().toString();
private static final String TEST_WORKSPACE = UUID.randomUUID().toString();
private static final String WORKSPACE_NAME = "workspace-" + UUID.randomUUID();

private static final RedisContainer REDIS = RedisContainerUtils.newRedisContainer();

Expand Down Expand Up @@ -93,7 +93,7 @@ void setUpAll(ClientSupport client, Jdbi jdbi) throws Exception {

ClientSupportUtils.config(client);

mockTargetWorkspace(API_KEY, TEST_WORKSPACE, WORKSPACE_ID);
mockTargetWorkspace(API_KEY, WORKSPACE_NAME, WORKSPACE_ID);

this.traceResourceClient = new TraceResourceClient(this.client, baseURI);
this.evaluatorResourceClient = new AutomationRuleEvaluatorResourceClient(this.client, baseURI);
Expand All @@ -117,20 +117,20 @@ class TracesCreatedEvent {
@DisplayName("when a new trace is created, OnlineScoring should see it within a event")
void when__newTracesIsCreated__onlineScoringShouldKnow() {
var projectName = factory.manufacturePojo(String.class);
var projectId = projectResourceClient.createProject(projectName, API_KEY, TEST_WORKSPACE);
var projectId = projectResourceClient.createProject(projectName, API_KEY, WORKSPACE_NAME);

var evaluator = factory.manufacturePojo(AutomationRuleEvaluatorLlmAsJudge.class)
.toBuilder().projectId(projectId).build();
.toBuilder().projectId(null).build();

evaluatorResourceClient.createEvaluator(evaluator, TEST_WORKSPACE, API_KEY);
evaluatorResourceClient.createEvaluator(evaluator, projectId, WORKSPACE_NAME, API_KEY);

var trace = factory.manufacturePojo(Trace.class).toBuilder()
.projectName(projectName)
.build();

UUID traceId = traceResourceClient.createTrace(trace, API_KEY, TEST_WORKSPACE);
UUID traceId = traceResourceClient.createTrace(trace, API_KEY, WORKSPACE_NAME);

Trace returnTrace = traceResourceClient.getById(traceId, TEST_WORKSPACE, API_KEY);
Trace returnTrace = traceResourceClient.getById(traceId, WORKSPACE_NAME, API_KEY);

// TODO: run the actual test checking for if we have a FeedbackScore by the end. Prob mocking AI Proxy.
}
Expand Down
Loading
Loading