-
Notifications
You must be signed in to change notification settings - Fork 231
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
[OPIK-218] Remove limitations in dataset items #369
[OPIK-218] Remove limitations in dataset items #369
Conversation
…' of https://github.com/comet-ml/opik into thiagohora/OPIK-218_remove_limitations_in_dataset_items
b9890a4
to
c8edc9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some minor and other no blocking comments. Let's focus and discuss:
- the rollout strategy.
- how co-existence of the old and new fields should be handled.
- considering multiple versions of the SDK released.
- considering the frontend expectations.
- the data migration strategy.
- fern generation for polymorphic types.
apps/opik-backend/src/main/java/com/comet/opik/api/DatasetItem.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/DatasetItemInputValue.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/DatasetItemInputValue.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/validate/DatasetItemInputValidator.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
...in/resources/liquibase/db-app-analytics/migrations/000004_add_input_data_to_dataset_item.sql
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetsResourceTest.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed and agreed this morning, this LGTM.
apps/opik-backend/src/main/java/com/comet/opik/api/DatasetItemInputValue.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetsResourceTest.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetsResourceTest.java
Show resolved
Hide resolved
8567fdf
to
260339f
Compare
72ced66
to
ad13f5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the latest revision has two major issues that we have to review:
- It's not returning the type of the columns, which is a requirement for FE per the ticket.
- It's resolving the type of the fields before storing them (which might be fine for caching purposes etc.) but then it isn't doing anything with them. The type of a field is implicit any valid JSON payload, so there's no need to resolve and store this extra information unless there's a reason to.
Finally, I left some guidance to easily accommodate the next requirements as filtering.
Once we're on the same page about these suggestions, following this approach is much easier as it's just a matter or removing some of the current code, like happened with the previous revision.
apps/opik-backend/src/main/java/com/comet/opik/api/DatasetItem.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/validate/DatasetItemInputValidator.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
From what @ferc said, the column types do not need to be returned yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last two issues were discussed, agreed and addressed. This is ready to go from my side.
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetsResourceTest.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
...in/resources/liquibase/db-app-analytics/migrations/000004_add_input_data_to_dataset_item.sql
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/validate/DatasetItemInputValidator.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemDAO.java
Outdated
Show resolved
Hide resolved
Agreed to move forward with at least the type in the returned payload. However, getting the type value was trivial, so the latest revision resolves it as well. |
Details
We will remove limitations in dataset items by deprecating the
input
,expected_output
andmetadata
to favor a more generic structure. The new fielddata
is a Dict[String, String] or Map<String, String>, allowing users to send any number of columns they want.To maintain backward compatibility (until we actually remove the old fields), the data from the
input
,expected_output
andmetadata
fields are automatically copied to the newinput_data
field. Also, a newcolumns
column was added to the Page object to show the frontend which columns are present in the dataset.The new input_data field has the following format:
DatasetItem model
Sample:
As per the page object, it will look like this:
Issues
#OPIK-218
Testing
Documentation
OPENAPI docs were updated to reflect new and deprecated columns.