-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Upload dataset from il sdk #1116
base: main
Are you sure you want to change the base?
Conversation
cbbb5a3
to
adc2e68
Compare
adc2e68
to
c934a18
Compare
Dataset, | ||
Example, | ||
ExpectedOutput, | ||
) | ||
|
||
|
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.
We should add a block comment somewhere that this is not exported to connectors.XXX. Maybe it would even be better to not export the StudioDataRepository to eval but rather export this to connectors
dataset: :Dataset: to be uploaded | ||
examples: :Examples: of Dataset |
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.
where did you get this docstring? i feel like we always did this via ...
instead of :...:
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.
Fair, had it mixed up. Corrected it
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.
with backticks i mean
_status_code_to_exception: ClassVar[dict[int, type[DataError]]] = { | ||
HTTPStatus.SERVICE_UNAVAILABLE: DataExternalServiceUnavailable, | ||
HTTPStatus.NOT_FOUND: DataResourceNotFound, | ||
HTTPStatus.UNPROCESSABLE_ENTITY: DataInvalidInput, | ||
HTTPStatus.FORBIDDEN: DataForbiddenError, | ||
} |
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 don't quite see the reason to make these our own errors
@fixture | ||
def examples() -> Sequence[Example[str, str]]: | ||
return [ | ||
Example(input="input_str", expected_output="output_str"), | ||
Example(input="input_str2", expected_output="output_str2"), | ||
] |
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.
have you checked if we have this fixture already? otherwise not a big deal to redefine it if it does not fit
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.
Yes, but in the "wrong" conftest.
) | ||
result = studio_client.submit_dataset(dataset=dataset, examples=examples) | ||
|
||
assert result is not None |
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.
we could check that the dataset we get back has the metadata / labels etc that we expect
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.
Added an "has been called with" assertion type
Description
PHS-842
Before Merging
changelog.md
if necessary