Skip to content

[sdk] Add to multifile dataset #13

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

Jump to bottom
Merged
merged 7 commits into from
Sep 4, 2024
Merged

[sdk] Add to multifile dataset #13

merged 7 commits into from
Sep 4, 2024

Conversation

alkasm
Copy link
Contributor

@alkasm alkasm commented Sep 4, 2024 •
edited
Loading

  • update API definitions (moves ingest/ and upload/ folders into combined/)
  • add method to append data to dataset
  • pass the nominal client everywhere (rather than each individual service client). wanted to stay pure here by only injecting the necessary dependencies but it just makes the code a lot uglier; can refactor in the future with a better pattern

TODO:

  • test on local instance
  • test on staging

@alkasm
add_to_dataset_from_io
3be85b8 
- update API definitions (moves ingest/ and upload/ folders into combined/)
- add method to append data to dataset
- pass the nominal client everywhere (rather than each individual client)
alxhill
alxhill reviewed Sep 4, 2024
View reviewed changes
alxhill
alxhill approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@alxhill alxhill left a comment

Choose a reason for hiding this comment

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

one nit, otherwise looks 🔥

@alkasm alkasm marked this pull request as ready for review September 4, 2024 22:20
@alkasm alkasm requested a review from alxhill September 4, 2024 22:20
alkasm
alkasm commented Sep 4, 2024
View reviewed changes
alxhill
alxhill reviewed Sep 4, 2024
View reviewed changes
alxhill
alxhill approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@alxhill alxhill left a comment

Choose a reason for hiding this comment

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

one note, otherwise lgtm

@alkasm alkasm merged commit d35105a into main Sep 4, 2024
1 check passed
@alkasm alkasm deleted the alkasm/multifile-datasets branch September 4, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alxhill alxhill alxhill approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alkasm @alxhill