Skip to content

Conversation

@zeke
Copy link
Member

@zeke zeke commented Mar 1, 2022

Does what it says on the tin.

Part of #407

zeke added 2 commits March 1, 2022 11:14
Signed-off-by: Zeke Sikelianos <[email protected]>
Signed-off-by: Zeke Sikelianos <[email protected]>
zeke added 2 commits March 1, 2022 14:07
Signed-off-by: Zeke Sikelianos <[email protected]>
Signed-off-by: Zeke Sikelianos <[email protected]>
@bfirsh
Copy link
Member

bfirsh commented Mar 1, 2022

A few notes:

  1. It might worth mentioning that File and Path are how you get files in and out of models. That's kind of implied, but not explicit.

  2. I wonder if it's worth wrapping the example with class Predictor(BasePredictor):? That might be confusing to someone not intimately familiar with Cog.

  3. It's a bit confusing how the File example uses Path as an input. I reckon File will more commonly be used as input than output. Most libraries make it easy to read from a file handle, easy to write to disk, but most don't expose a file handle. E.g. it's easy to read a file from a file handle with Pillow (Image.open(fh)) but it's not clear how to get the file handle back out of it again. (Maybe the object itself is a file handle? I'm not sure...)

  4. I wonder whether we could have a more real example to illustrate what's going on? E.g.

    from PIL import Image
    
    class Predictor(BasePredictor):
        def predict(self, image: File):
             img = Image.open(image)
             ...

    (not sure if this actually works...)

based on feedback from Benzo

Signed-off-by: Zeke Sikelianos <[email protected]>
@zeke
Copy link
Member Author

zeke commented Mar 1, 2022

Good feedback! Made some updates. This is ready for another look.

@bfirsh bfirsh merged commit 2540092 into future Mar 1, 2022
@bfirsh bfirsh deleted the document-cog-file-and-cog-path branch March 1, 2022 23:49
bfirsh pushed a commit that referenced this pull request Mar 1, 2022
* document cog.Path()

Signed-off-by: Zeke Sikelianos <[email protected]>

* document cog.File()

Signed-off-by: Zeke Sikelianos <[email protected]>

* fix import

Signed-off-by: Zeke Sikelianos <[email protected]>

* whoops don't forget Input

Signed-off-by: Zeke Sikelianos <[email protected]>

* update docs for cog.File and cog.Path

based on feedback from Benzo

Signed-off-by: Zeke Sikelianos <[email protected]>
zeke added a commit that referenced this pull request Mar 2, 2022
* document cog.Path()

Signed-off-by: Zeke Sikelianos <[email protected]>

* document cog.File()

Signed-off-by: Zeke Sikelianos <[email protected]>

* fix import

Signed-off-by: Zeke Sikelianos <[email protected]>

* whoops don't forget Input

Signed-off-by: Zeke Sikelianos <[email protected]>

* update docs for cog.File and cog.Path

based on feedback from Benzo

Signed-off-by: Zeke Sikelianos <[email protected]>
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