Skip to content

Conversation

etai-shuchatowitz
Copy link
Member

No description provided.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jun 26, 2025
@@ -50,6 +50,8 @@ type Service interface {
resource.Resource
// Sync will sync data stored on the machine to the cloud.
Sync(ctx context.Context, extra map[string]interface{}) error
UploadImageToDataset(ctx context.Context, image []byte, datasetIDs, tags []string,
Copy link
Member

Choose a reason for hiding this comment

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

don't we need the mime type?
can we also have an image.Image version even if just for the go sdk?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes! we definitely should have the MIME type. Lemme add that.

Do you want the image.Image version instead or in addition? If we pass in MIME type then it’s not an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd like both.

s.logger.Errorw("error reading file", "err", err)
return err
}
s.syncArbitraryFile(f, tags, datasetIDs, 0, s.logger)
Copy link
Member

Choose a reason for hiding this comment

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

is this sync or async?
if we're writing to disk anyway, we can make async.
otherwise, let's not write to disk

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be async. The reason to write to disk is because all of our functionality is written to take in a os.File and in the name of not rewriting everything just to account for this, thought it would be easier just to write to disk and delete it afterwards.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just mark it as TODO please.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants