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

feat: support for list in inputs #1561

Merged
merged 6 commits into from
Mar 25, 2024
Merged

feat: support for list in inputs #1561

merged 6 commits into from
Mar 25, 2024

Conversation

dkhokhlov
Copy link
Contributor

@dkhokhlov dkhokhlov commented Mar 6, 2024

$ echo test1 > 1.txt
$ echo test2 > 2.txt
$ cat predict.py
from cog import BasePredictor, Path

class Predictor(BasePredictor):
   def predict(self, paths: list[Path]) -> str:
       output_parts = [] 
       for path in paths:
           with open(path) as f:
             output_parts.append(f.read())
       return "".join(output_parts)

$ cog predict -i paths=@1.txt -i paths=@2.txt

Running prediction...
test1

test2
  • Note the repeated inputs with the same name "paths" which constitute the list

Ref:

@dkhokhlov dkhokhlov force-pushed the support_for_list_in_input branch 2 times, most recently from f02b3e9 to 641da4c Compare March 6, 2024 06:14
@dkhokhlov dkhokhlov requested a review from mattt March 6, 2024 06:17
@dkhokhlov dkhokhlov changed the title Support for list in inputs WIP: support for list in inputs Mar 6, 2024
Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
@dkhokhlov dkhokhlov changed the title WIP: support for list in inputs support for list in inputs Mar 7, 2024
@dkhokhlov dkhokhlov marked this pull request as ready for review March 7, 2024 16:46
@dkhokhlov dkhokhlov requested review from a team and technillogue March 7, 2024 16:46
…ct -> path-project, as it is about path

Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
@dkhokhlov dkhokhlov requested review from nickstenning and a team and removed request for a team March 8, 2024 03:06
@dkhokhlov dkhokhlov self-assigned this Mar 8, 2024
@dkhokhlov dkhokhlov changed the title support for list in inputs feat: support for list in inputs Mar 9, 2024
@zeke
Copy link
Member

zeke commented Mar 12, 2024

This is awesome, @dkhokhlov!

Can you take a shot at documenting this in docs/python.md ?

Let me know if you want help with that.

@dkhokhlov dkhokhlov requested a review from yorickvP March 15, 2024 05:17
Copy link
Member

@nickstenning nickstenning left a comment

Choose a reason for hiding this comment

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

I haven't finished going through this yet, but I figure I should release the comments I've made already to get started :)

"github.com/vincent-petithory/dataurl"

"github.com/mitchellh/go-homedir"
Copy link
Member

Choose a reason for hiding this comment

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

This import moved, and it should move back :)

@@ -3,6 +3,7 @@

This prints a JSON object describing the inputs of the model.
"""

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

@dkhokhlov dkhokhlov Mar 21, 2024

Choose a reason for hiding this comment

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

this change is required by one of new linters in make lint:
python -m ruff format --check python
and reformatted by:
python -m ruff format python/cog/command/openapi_schema.py

webhook_events_filter: t.Optional[
t.List[WebhookEvent]
] = WebhookEvent.default_events()
webhook_events_filter: t.Optional[t.List[WebhookEvent]] = (
Copy link
Member

Choose a reason for hiding this comment

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

Changes like this suggest that you don't have your editor set up to do autoformatting the same as script/format. Would you look into that?

Copy link
Contributor Author

@dkhokhlov dkhokhlov Mar 21, 2024

Choose a reason for hiding this comment

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

this change is required by one of new linters in our make lint:
python -m ruff python/cog
and reformatted by:
python -m ruff format python/cog/schema.py

)

type Input struct {
String *string
File *string
Array *[]interface{}
Copy link
Member

Choose a reason for hiding this comment

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

Two small points:

  1. it's nice to use any rather than interface{} these days
  2. this doesn't need to be a pointer -- slices are already pointer types

Comment on lines 77 to 91
var dataURLs []string
for _, elem := range *input.Array {
if str, ok := elem.(string); ok && strings.HasPrefix(str, "@") {
filePath := str[1:] // Remove '@' prefix
dataURL, err := fileToDataURL(filePath)
if err != nil {
return keyVals, err
}
dataURLs = append(dataURLs, dataURL)
} else if ok {
// Directly use the string if it's not a file path
dataURLs = append(dataURLs, str)
}
}
keyVals[key] = dataURLs
Copy link
Member

Choose a reason for hiding this comment

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

It's not a big deal, but it would be more conventional when you know the length of the slice beforehand to do something like

Suggested change
var dataURLs []string
for _, elem := range *input.Array {
if str, ok := elem.(string); ok && strings.HasPrefix(str, "@") {
filePath := str[1:] // Remove '@' prefix
dataURL, err := fileToDataURL(filePath)
if err != nil {
return keyVals, err
}
dataURLs = append(dataURLs, dataURL)
} else if ok {
// Directly use the string if it's not a file path
dataURLs = append(dataURLs, str)
}
}
keyVals[key] = dataURLs
dataURLs := make([]string, len(input.Array))
for i, elem := range input.Array {
if str, ok := elem.(string); ok && strings.HasPrefix(str, "@") {
filePath := str[1:] // Remove '@' prefix
dataURL, err := fileToDataURL(filePath)
if err != nil {
return keyVals, err
}
dataURLs[i] = dataURL
} else if ok {
// Directly use the string if it's not a file path
dataURLs[i] = str
}
}
keyVals[key] = dataURLs

} else if strings.HasPrefix(val, "[") && strings.HasSuffix(val, "]") {
// Handle array of strings
var arr []interface{}
if err := json.Unmarshal([]byte(val), &arr); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Is JSON unmarshalling right here? That means the strings have to be quoted, which is inconsistent with the other types.

For example, I can do

cog predict hello-world -i text=world

or

cog predict resnet -i image=@input.jpg

but with this implementation, the following will throw an error

cog predict mymodel -i names=[foo,bar]

I think there are two possibilities here. Either:

  1. do the work to allow individual strings to be either quoted or unquoted, so that they're consistent with the rest of the interface, or
  2. change approach to do arrays by just repeating inputs, e.g. cog predict ... -i names=one -i names=two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose repeated inputs. Good idea. Thanks.

@dkhokhlov dkhokhlov force-pushed the support_for_list_in_input branch 6 times, most recently from dd10b81 to 1b36ac1 Compare March 21, 2024 02:34
Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
Signed-off-by: Dmitri Khokhlov <dkhokhlov@gmail.com>
@dkhokhlov
Copy link
Contributor Author

dkhokhlov commented Mar 21, 2024

Can you take a shot at documenting this in docs/python.md ?

Let me know if you want help with that.

@zeke
added new section about List type with cog cli example in docs/python.md:
https://github.com/replicate/cog/pull/1561/files#diff-3576f8388f597edf93166fb888bc0c6c7ebca8aba465d941c17b3c1804b99cab
please feel free to polish/add more.

@dkhokhlov dkhokhlov merged commit 1289b6f into main Mar 25, 2024
14 checks passed
@dkhokhlov dkhokhlov deleted the support_for_list_in_input branch March 25, 2024 20:34
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.

None yet

3 participants