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.

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

feature: Creation of connectors to Studio(Dataset, Runner, Evaluation and Aggregation) #1003

Closed
wants to merge 9 commits into from

Conversation

mveleci
Copy link
Contributor

@mveleci mveleci commented Aug 21, 2024

Description

  • Add DataClient and StudioDatasetRepository as connectors to Studio for submitting data.
  • Add StudioRunnerRepository as a connector to Studio for submitting runs.
  • Add StudioEvaluationRepository as a connector to Studio for submitting evaluations.
  • Add StudioAggregationRepository as a connector to Studio for submitting aggregations.

Before Merging

  • Review the code changes
    • Unused print / comments / TODOs
    • Missing docstrings for functions that should have them
    • Consistent variable names
  • Update changelog.md if necessary
  • Commit messages should contain a semantic label and the ticket number
    • Consider squashing if this is not the case

Copy link
Collaborator

Choose a reason for hiding this comment

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

This implementation will not work at all. The entire point of this whole exercise is that anyone can access the data from the data platform. However, the methods are not implemented to access the data from the platform, instead, they access the data from the local file system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same holds for all repositories.

Comment on lines +10 to +11
class Config:
alias_generator = to_camel
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to match the Pydantic 2 suggested approach? Something like:

model_config = ConfigDict(alias_generator=AliasGenerator(serialization_alias=to_camel))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this at all? Can't we use the normal BaseModel?

Comment on lines +80 to +84
429,
500,
502,
503,
504,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to use the codes from HTTPStatus?

@NickyHavoc NickyHavoc force-pushed the DATA-117-data-client branch from 290ed53 to 62136bb Compare August 21, 2024 13:27
@mveleci mveleci closed this Aug 21, 2024
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.

3 participants