FEAT generalize OpenAISoraTarget to be compatible with Sora2 #1126
Conversation
| n_seconds: Optional[int] = None, | ||
| n_variants: Optional[int] = None, | ||
| api_version: Optional[str] = None, | ||
| resolution_dimensions: str = "1280x720", |
There was a problem hiding this comment.
I have Bashir's voice in my head saying make it Optional 👻 Any reason why we cannot keep it Optional[str] and Optional[int] here and then set it to the default values in the body?
There was a problem hiding this comment.
Optional makes a lot of sense for objects or lists, not so much for int/str unless you want to allow None which we don't here.
There was a problem hiding this comment.
Also, this would actively obscure what the default is. Imagine reading the API reference and not being able to tell unless you read the entire arg description (where we hopefully document it)
There was a problem hiding this comment.
Gotcha - just making sure we have consistent convention across the codebase but I haven't looked at other places where we use Optional vs pre-set default values. Using Optional is fairly new to me so maybe I'm mapping it wrong in my head. @hannahwestra25 Do you have thoughts as the person who went through and did this pattern enforcement most recently?
romanlutz
changed the title
romanlutz
changed the title
…tests, unit tests, and documentation improvements.
| Raises: | ||
| ValueError: If video constraints are not met for the specified resolution. | ||
| Note: DO NOT SET if using target with PromptSendingAttack as multiple files will be generated |
There was a problem hiding this comment.
I am a little confused at when you would use this? E.g. why not PromptSendingAttack and the response is all maybe?
There was a problem hiding this comment.
I understand that only certain configurations support n_variants. And I understand this param asks sora to create variants. But I'm not sure of the target's behavior when n_variants is more than 1 or when I would/wouldn't use it.
The ask here might just be to document better?
There was a problem hiding this comment.
N_variants is a sora1 feature only. And even then it works only in some cases. The file name arg is to set the output file name which is a problem if you run more than one prompt or n_variants>1. I don't think we should have it at all but am reluctant to remove something that people may have put here for a reason. There's a separate thread where @nina-msft explained the purpose.
There was a problem hiding this comment.
Maybe let's just remove this since sora-2 doesn't support it and sora-1 will be phased out eventually.
6 participants
Description
First version of a target for SORA2.
Tests and Documentation