clamsproject / clams-python

CLAMS SDK for python
http://sdk.clams.ai/
Apache License 2.0
4 stars 1 forks source link

handing over additional parameters to an app #29

Closed keighrim closed 1 year ago

keighrim commented 3 years ago

When assuming a CLAMS app is running only as a Python object, the app developer has more than a million ways to implement parameterized options that can be configured for calling annotate - additional public methods, class variables/constants, etc. However when an app runs as a HTTP server - as ones do in appliance - there's not many options to configure the app for additional parameters for annotate method. Here, I'm proposing an implementation of parameter as URL query string.

For example, we want to run Tesseract on views that generated by a specific slate detector only if we are very sure that in the pipeline, that slate detector app is in somewhere in upstream. Otherwise we want to run Tesseract on everything the app can digest. In this case, we can write the Tesseract wrapper app only once, and make the annotate method can take an optional parameter, for exmaple, target_view_producer (defaults to None). We then specify BoundingBox annotation type as an optional input in the Tesseract app metadata.

URL string format

When considering passing such parameters via URL string, there are some restrictions we need to take into account.

  1. length limit - Technically there's no limits on the length of a URL, yet different web browsers use their own limits. However though, in most cases, the limit is larger than a couple thousands bytes, so I think we won't hit such limits and don't have to worry about it.
  2. URL encoding - Only certain characters can consist URL strings. Thus all keys and possible values must meet the URL scheme. For keys, we don't have to worry as python's restrictions on identifier names are much stricter than those of URL's. However for values, this restriction can pose serious limits. For example, what if an app wants to accept an AnnotationType value? Because AnnotationTypes themselves are already URI/IRI's, we need carefully encode and decode special characters like /.

At annotate

These parameters are only for annotate method. Hence, URLs for PUT requests can have them. In restify.Restifier, we can decode those parameters and pass them when the annotate Python method is called.

At AppMetadata and docstring of annotate

So the question is where and how users can obtain information of those parameter. I'm thinking that the parameter names and short descriptions should be specified in the metadata of the app. The app metadata is the most natural way for users to obtain information about the app. One beautiful thing about Python is that, in this scenario, a developer can write the parameter specification in the docstring of the annotate method, and in setupmetadata, s/he can call self.annotate.__doc__ to get the specification and convert into metadata format.

At appliance and Galaxy

Appliance maker should query the app metadata once the code is cloned to local system. I'm thinking that the maker can do something like this; clone --> docker build --> docker run --> GET request --> docker stop . Once the app metadata is returned, maker can generate tool_conf.xml in a way that exposes those parameters in the web GUI.

Example

# we have this app 
class TesseractOCR(ClamsApp): 
    ...
    def annotate(self, mmif, target_view_producer: str):
        ... 
        for view in mmif: 
            if view.metadata.app == target_view_producer: 
                do something
            else:
                do_nothing
        ...
    ...

When asked for its metadata,

>>> app = TesseractOCR()
>>> print(app.appmetadata())

the app returns;

{
  "name": "edu.brandeis.clams.tesseract_ocr", 
  "version": "1.1.7", 
  "required": 
  { 
    "http://mmif.clams.ai/1.0.3/vocabulary/VideoDocument/": { "optional": false }, 
    "http://mmif.clams.ai/1.0.3/BoundingBox": { "optional": true }
  }, 
  "parameters": 
  {
    "target_view_producer": "ID of the app which generated views with BoundingBox annotations"
  }
  ...
}

Suppose that the app is running as HTTP server on localhost:5000. Then we can send a request like this one.

curl -X PUT -d some_mmif.json localhost:5000/?required_types=http%3A%2F%2Fmmif.clams.ai%2F1.0.3%2FBoundingBox&target_view_producer=edu.brandeis.clams.slate_detector 

Note that all optional input types can now be toggled by passing the required_types parameter. I'm not sure whether this is going to work when there's a need to turn on multiple optional at_types. We might need to give more thoughts on how to turn on optional at_types.

In galaxy tool_conf.xml we can automatically add these elements;

...
<inputs>
  ...
  <param label="ID of the app which generated views with BoundingBox annotations" name="target_view_producer" type="text">
  <param label="Expected optional at_types" name="required_types" type="select" multiple="true" display="checkboxes">
    <option value="http%3A%2F%2Fmmif.clams.ai%2F1.0.3%2FBoundingBox">http://mmif.clams.ai/1.0.3/BoundingBox</option>
  ...
</inputs>
...
<command>
  curl -X PUT -d @$input_mmif tesseractocr:5000/?required_types=$required_types&target_view_producer=$target_view_producer &gt $output
</command>

Let me know how you think.

marcverhagen commented 3 years ago

A few notes on this.

Implementation. Is it correct to assume that you want to set up ClamsRestfulApi.put() so that it takes the parameters and and then hands them as key word arguments into the application's annotate()?

Exposing parameters. Yes, I totally agree this is best done in the metadata and that it should be returned by the GET method and appmetadata(). It is up to the developer to decide how to do it, you can use a docstring, but I personally would put it straight into setupmetadata() and I would like it if developers can always find metadata at the same place in the code.

URLs in parameters. Do we need that? I think we still do not know how to do the requires stuff and now this would partially use parameters for that. I would be happier with parameters like use_textbox=True or use__east_textbox=True and then let the sniff() or annotate() method decide where to look for the input.

Galaxy config. I find the whole thing of docker building and running those applications to be a bit heavy. It would be nice if we could pull that information straight from the code by running appmetadata(), but we then need to know the name of the class,which is not rocket science but would put a tiny extra burden on the developer. We could also choose to have the metadata be defined in a global variable and we then ditch setupmetadata().

keighrim commented 3 years ago

Implementation. Is it correct to assume that you want to set up ClamsRestfulApi.put() so that it takes the parameters and and then hands them as key word arguments into the application's annotate()?

Correct. Exposing parameters. Yes, I totally agree this is best done in the metadata and that it should be returned by the GET method and appmetadata(). It is up to the developer to decide how to do it, you can use a docstring, but I personally would put it straight into setupmetadata() and I would like it if developers can always find metadata at the same place in the code.

I'd personally like to keep a single and only place to store the information. Unless I decide not to write the docstring at all (which is also quite likely), I'd do the __doc__ trick. But as you said, it's completely up to the app developers.

URLs in parameters. Do we need that? I think we still do not know how to do the requires stuff and now this would partially use parameters for that. I would be happier with parameters like use_textbox=True or use__east_textbox=True and then let the sniff() or annotate() method decide where to look for the input.

In the long run, assuming we implement the app directory where all apps are assigned with IRI-like identifiers (based on the public repository the app is hosted) and we don't re-introduce JSON-LD context for nicknames , I think it is important to use IRI form of annotation/document types and app ids, to keep the consistency of individual MMIF output and transparency of the platform. For example, use__east_textbox parameter is actually implying for three things, 1) existence of views where there are BoundingBox annotations 2) existence of views where there are BoundingBox annotations are and the boxType properties of some of those are textbox, and 3) existence of views where there are BoundingBox annotations are and the boxType properties of some of those are textbox and app metadata of the view is https://what.ever.east.app.id.is . That level of abstraction seriously concerns me, and I think each of them needs to be separately specified as independent parameters.

I agree that we don't know how to do the requires at the moment - for example, whether an app can require something at type-level, metadata-level, or property-level. This issue is an attempt to start solving that problem.

Galaxy config. I find the whole thing of docker building and running those applications to be a bit heavy. It would be nice if we could pull that information straight from the code by running appmetadata(), but we then need to know the name of the class,which is not rocket science but would put a tiny extra burden on the developer. We could also choose to have the metadata be defined in a global variable and we then ditch setupmetadata().

Not only we need to know the name of the class, we also need to know the list of dependencies and install all the dependency before we can run the code locally (not in a container). Which is still not a rocket science, but could go pretty messy very easily, even with virtualenv (e.g. the officially recommended way to install numpy - which is essential for almost all machine learning apps - on Windows is installing conda first and use it to install numpy). setupmedata() is there so that developers can configure some values programmatically (for instance, when they want to keep a single source of version value across the app like us using the VERSION file.) For Galaxy appliance or any other workflow engine under development, I think we are assuming that all apps are running as containers at the end of the day, so the time spent on building images to get the app metadata won't be wasted.

keighrim commented 3 years ago

On the last point, we can also have an appmetadata published in the app directory, so that appliance or other clams builder can simply get information from a publish website. More details are relying on the implementation of the app directory.

marcverhagen commented 3 years ago

Not only we need to know the name of the class, we also need to know the list of dependencies and install all the dependency before we can run the code locally (not in a container). Which is still not a rocket science, but could go pretty messy very easily, even with virtualenv (e.g. the officially recommended way to install numpy - which is essential for almost all machine learning apps - on Windows is installing conda first and use it to install numpy). setupmedata() is there so that developers can configure some values programmatically (for instance, when they want to keep a single source of version value across the app like us using the VERSION file.) For Galaxy appliance or any other workflow engine under development, I think we are assuming that all apps are running as containers at the end of the day, so the time spent on building images to get the app metadata won't be wasted.

Yes, more complicated than I would like indeed. When you put it that way doing the docker build etcetera does not seem so bad compared to running some Python code. One option that we do have is to adopt an approach where we have a JSON or YAML file with the metadata and this is read automatically when initializing the app.

marcverhagen commented 3 years ago

On the last point, we can also have an appmetadata published in the app directory, so that appliance or other clams builder can simply get information from a publish website. More details are relying on the implementation of the app directory.

Yeah, but are we then introducing a chicken and egg problem? I am not sure it does, but I do feel a vague sense of trouble here. Will this force us to put an app in the app directory (@keighrim you are promoting this name somewhere else and I see nothing wrong with it) before we can get metadata from it?

marcverhagen commented 3 years ago

In the long run, assuming we implement the app directory where all apps are assigned with IRI-like identifiers (based on the public repository the app is hosted) and we don't re-introduce JSON-LD context for nicknames , I think it is important to use IRI form of annotation/document types and app ids, to keep the consistency of individual MMIF output and transparency of the platform. For example, use__east_textbox parameter is actually implying for three things, 1) existence of views where there are BoundingBox annotations 2) existence of views where there are BoundingBox annotations are and the boxType properties of some of those are textbox, and 3) existence of views where there are BoundingBox annotations are and the boxType properties of some of those are textbox and app metadata of the view is https://what.ever.east.app.id.is . That level of abstraction seriously concerns me, and I think each of them needs to be separately specified as independent parameters.

It is true that use_east_textbox implies those things. But I do not see that abstraction as a problem, on the contrary, I see it as a strength. You are absolute right that in the end any reference to EAST should be grounded in the URI in the app directory and I think that can be easily done by documenting the parameters. The decomposition approach forces a user to explicitly state the three implications you mention. It pushes complexity to the user and the user will have to know exactly what is going on under the hood. If I were a user I would prefer saying "I want Tesseract to run on EAST output" instead of "I want views with BoundingBox annotations and I want those to have boxType=text and I want those created by https://what.ever.east.app.id.is.

keighrim commented 3 years ago

7b231c49 is a proof-of-concept implementation of parameter passing using URL query. Note that it is a breaking change that effect ALL existing apps. Specifically, annotate method now should take another ** argument and should return Mmif object instead of a str serialized from the Mmif obj. (see these lines).

Also note, in addition to that, the annotate method is now wrapped in annotate_and_serialize method, and http PUT verb is bound to annotate_and serialize. I'd prefer renaming the old abstract method annotate to _annotate and have the new public wrapper method as simply annotate to changing the signature of the abstract annotate method, as it seems to be less breaking.

Let me know how you think.

keighrim commented 3 years ago

A new branch from the latest develop is now pushed with commits that have changes we discussed yesterday's meeting.

keighrim commented 3 years ago

Another issue popped at today's meeting;

When an OCR app takes

  1. a full video document

and optionally can handle one of these

  1. timeframes marked as slate (optional)
  2. timeframes marked as ending-credit (optional)
  3. boundingboxes marked as text (optional)

A user of the app can specify to use existing slate annotations and/or ending-credit annotations.

Primary problem arises from this example is that we don't know (yet) how to specify that in the appmetadata; one exemplary json was proposed as

parameters: {
    "run-on-timeframe": {
        "values": ["slate", "credits"],
        "description": "Tessearact to run on ..."},
    "run-on-bounding-boxes": {
        "values": ["text"],
        "description": "Tessearact to run on ..."}
} 

The value field has a list but we don't know whether we need to interpret the list as conjunctive or disjunctive.

marcverhagen commented 3 years ago

The list is neither a conjunction or a disjunction, it is just a list of all options (which makes it disjunctive I guess).

The question is whether we allow the actual parameter value to be a conjunction or disjunction of the list, we could have slate|credits or slate&credits (the latter makes little sense here though). Another option is add all as a value, or some other atomic value that makes sense for the app.

I am still not enamored with

"requires": [ 
    { "@type": "VideoDoc", "required": true },
    { "@type": "TimeFrame", "required": false, "type": "credit" },
    { "@type": "TimeFrame", "required": false, "type": "slate" },
    { "@type": "BoundingBox", "required": false, "boxType": "text" } ]

What if there are 20 options? Do we add 20 requires elements? Do we change the value of "type" to a list? There is some nice simplicity on just using simple attribute value pairs. I see some simplicity in having requires just setting the playing field, just saying that it wants video documents and can do something with time frames and boxes of a particular kind.

"requires": [ 
    { "@type": "VideoDoc", "required": true },
    { "@type": "TimeFrame", "required": false },
    { "@type": "BoundingBox", "required": false, "boxType": "text" } ]

And then the parameters specify this further, so run-on-timeframe=slate says slate frames are the input and run-on-bounding-boxes=text says it wants text boxes.

What I do not like there though is that box type is text is specified on two places, so maybe we want to ditch the box type on the requires:

"requires": [ 
    { "@type": "VideoDoc", "required": true },
    { "@type": "TimeFrame", "required": false },
    { "@type": "BoundingBox", "required": false } ]

or use run-on-bounding-boxes=True.

keighrim commented 3 years ago

I think the structure of the input specification is pretty well defined in #50. What we don't have is the structure of runtime parameters. Here's what we need to decide specifically.

  1. Object structure of the individual parameter: I think we have consensus on the minimal information to encode - name, values, description
    1. name - the parameter name used in the url query
    2. values - possible values the the parameter can take
    3. description - human-friendly explanation on what the parameter does (for documentation)
  2. Possible data types for parameter values: so far we have boolean (e.g. pretty), string (e.g. timeframe types in the above example), and integer (e.g. slateapp). Do we want/need to add more data type? Also a related question is whether we want to encode the type of the value in the parameter object. This questions also leads to the next point.
  3. Structure of values field: in the above example with TimeFrame, the values field is just a list of strings. However there are parameters that we can't finitely iterate possible values in a single list (e.g. a parameter to be any positive integer). In such case we can leverage on a more commonly used schema to specify data types and ranges (such as json-schema). In that case, the values (or singular value ?) field can be a list or an object specifying data type and range.

With consideration for all points above, here's a fictional example for parameters for a OCR app.

... 
parameters: {
  "run-on-timeframe": {
    "values": ["slate", "credits"],
    "description": "make app to run only on time frames previously annotated as `slate` or `credits`."},
  "sampleRatio": {
    "values":   { 
      "type": "number",
      "minimum": 1
    },
    "description": "set sampling rate per second for extract frames"},
  "merge": {
    "values": { 
      "type": "boolean" 
    },
    "description": "if consecutive frames are identical, only outputs the first of them"},

  and maybe more...

} 
... 

Also worth noting that parameters for optional input types can be automatically generated (see https://github.com/clamsproject/clams-python/pull/53#issuecomment-823986166), once we decide the naming scheme for them.

marcverhagen commented 3 years ago
{
  "run-on-timeframe": {
    "values": {
      "type": "string",
      "choices": ["slate", "credits"],
      "default": "slate",
    },
    "description": "make app to run only on time frames previously annotated as `slate` or `credits`."
  }
}

choices is optional, can be empty. default is optional, and if it's not set, it means the parameter is optional and is not set (in the above example, it should mean NOT to run-on-timeframe, namely, do not use previous TimeFrame annotations). type is required and is "number", "string", "boolean" and maybe some others, no default. description is required.

keighrim commented 3 years ago

Current implementation (f27c318da3af814b42df9faec0661534616bedc8, in #53) is slight different from what's proposed in the right above in that;

  1. parameters is a list, not a dict
  2. each parameter object has name, description and value (not values) fields.
  3. the value object has datatype (not type), choices, and default

See this for an example under currently implementation.

marcverhagen commented 3 years ago
[
   {
     "name": "run-on-timeframe",
     "type": "string",
     "choices": ["slate", "credits"],
     "default": "slate",
     "description": "make app to run only on time frames previously annotated as `slate` or `credits`."
  }
]
keighrim commented 3 years ago

5d9b8762 (in #53) changes previous RuntimeParameter structure into the above.

keighrim commented 3 years ago

Considering all recent updates in I/O specification (#50, #51) , when an app can optionally process TimeFrame::type=slate or TimeFrame::type=credits, it can state that in its metadata as

...
"input": [
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "slate" }
  },
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "credits" }
  }
],
"parameters": 
  {
    "name": "run-on-timeframe",
    "type": "string",
    "choices": ["slate", "credits"],
    "default": "slate",
    "description": "make app to run only on time frames previously annotated as `slate` or `credits`."
  }
...

Then when a user wants this app to run both on slate and credits, one can maybe send a POST like

https://someapp/?run-on-timeframe=slate&run-on-timeframe=credits

Now we need to decide, when such a request comes in, whether the SDK should aggregate all values under the same name (run-on-timeframe=["slates", "credits"]) or should take only last seen value (run-on-timeframe="credits"). The former makes more sense from the perspective of users, but in terms of the SDK, it is contradicting the app metadata as the metadata clearly says the value must be a string, not a list of strings. Allowing this behavior and implementing handling of such cases will pose a significant work on the development of the SDK. On the other hand, with the latter behavior, it's not so user-friendly as the former, but users still can achieve their goal by calling the app twice with different parameter values (though it can be fairly inefficient and time-consuming, depend on the internal implementation of the app).

Given that, I'd like to go back to comments in the thread from February 2021, and want to throw some thoughts.

First, in the previous comment we thought the list of possible values was neither conjunctive nor disjunctive, but with its new name ("choices"), I think it is now more clear that the list is disjunctive. I also think that a situation like what I described above points to the same conclusion.

Next, regarding this question (https://github.com/clamsproject/clams-python/issues/29#issuecomment-772793820);

What if there are 20 options? Do we add 20 requires elements? Do we change the value of "type" to a list?

With the latest proposal (#50) to specify inputs (formerly requires), I think the answer now is that we add 20 disjunctive elements to the input list. Also based on that proposal I don't think it's a good idea to under-specify the input metadata and delegate property-level specification to the value from a runtime parameter. Namely I don't want an app manifests itself as;

...
"input": [
  { "@type": "TimeFrame", 
    "required": false
  }
],
"parameters": 
  {
    "name": "run-on-timeframe",
    "type": "string",
    "choices": ["slate", "credits"],
    "default": "slate",
    "description": "make app to run only on time frames previously annotated as `slate` or `credits`."
  }
...

(However I don't think it's possible that we validate or approve apps based on the specificity level shown in their metadata, either manually or automatically. Namely we probably can't prevent app developers from doing the above.)

Finally, combining all above discussion, I'd like to propose that a runtime parameter for picking optional inputs should limit its scope only to a single optional element from the input list of the app metadata. Namely, with the above example with slate and credits, the app declares two separate boolean runtime parameters, one for the slates and the other for credits. So its metadata will be something like this;

...
"input": [
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "slate" }
  },
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "credits" }
  }
],
"parameters": 
  {
    "name": "run-on-TimeFrame-frameType-slate",
    "type": "boolean",
    "description": "make app to use existing https://.../TimeFrame (frameType=slate) annotations"
  },
  {
    "name": "run-on-TimeFrame-frameType-credits",
    "type": "boolean",
    "description": "make app to use existing https://.../TimeFrame (frameType=credits) annotations"
  }
...

(we can maybe set a policy on how to name these "optional input" parameters consistently)

With this parameter set, user can now send a request like this

https://someapp/?run-on-TimeFrame-frameType-slate=true&run-on-TimeFrame-frameType-credits=true

to run the app both on slates and credits. Note that this proposal does not solve the issue with multiple values passed under the identical parameter name in general. But it at least systematically prevents such issues for optional input types.

A downside of this proposal is increased verbosity of the app metadata (and maybe elongated names of parameters). But I think the clarity it can provide outweighs some more bytes printed out. @kelleyl you have implemented apps with optional input types. Could you share your thoughts on how to formulate optional types in the parameters metadata?

keighrim commented 3 years ago

The parameter specification is implemented via #53, but I'm still waiting for inputs from people on optional input types and their corresponding runtime parameter.

kelleyl commented 3 years ago

I agree that the increased verbosity is a downside to the strategy of having a parameter for every input. I do like that it seems like the parameters could be automatically generated from the input, maybe requiring some additional information. Right now for the slate pipeline I think the metadata for tesseract would look something like:

...
"input": [
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "slate" }
  },
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "credits" }
},
 { "@type": "BoundingBox", 
    "required": false,
    "properties":  { "boxType": "text" }
  }
],
"parameters": 
  {
    "name": "run-on-TimeFrame-frameType-slate",
    "type": "boolean",
    "description": "make app to use existing https://.../TimeFrame (frameType=slate) annotations"
  },
  {
    "name": "run-on-TimeFrame-frameType-credits",
    "type": "boolean",
    "description": "make app to use existing https://.../TimeFrame (frameType=credits) annotations"
  },
  {
    "name": "run-on-BoundingBox-boxType-text",
    "type": "boolean",
    "description": "make app to use existing https://.../BoundingBox (boxType=text) annotations"
  }
...

One concern is if someone runs a pipeline of "slate detection -> text detection -> credit detection -> tesseract". In this scenario I think its unclear if the app should apply tesseract to full frames in the credits or not apply to the credits at all. Also, here are we assuming that the options for property keys and values are fixed? If a new tool is added that produces a new type for frameType, will downstream apps need to be updated to allow that frame type as a parameter?

marcverhagen commented 3 years ago

This is more involved than I thought and let's discuss this in person on Wednesday with a whiteboard. A few thoughts and comments.

Tesseract runs on frames of type "slate" and frames of type "credits".

I really don't like this:

"input": [
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "slate" }
  },
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": "credits" }
  }
]

But I have to reluctantly agree that with our current design of the input field this may be the best we have. It would be nicer to do it as follows, but that requires us to change the definition of what the input is.

"input": [
  { "@type": "TimeFrame", 
    "required": false,
    "properties":  { "frameType": ["slate", "credits"] }
  }
]

Keigh's objection

Namely I don't want an app manifests itself as;

"input": [
 { "@type": "TimeFrame", 
   "required": false
 }
],
"parameters": 
 {
   "name": "run-on-timeframe",
   "type": "string",
   "choices": ["slate", "credits"],
   "default": "slate",
   "description": "make app to run only on time frames previously annotated as `slate` or `credits`."
 }

Is the objection against not having the frameType=text part in the input? I agree with that. Or is it to the choices list. I do not agree with that objection. Basically an objection like that takes away the possibility of parameters with string values (at least in this case) and we end up with boolean thingies like run-on-timeframe-credits and run-on-timeframe-slate.

From POST to parameter dictionary

Then when a user want this app to run both on slate and credits, one can maybe send a POST like

https://someapp/?run-on-timeframe=slate&run-on-timeframe=credits

Now we need to decide, when such a request comes in, whether the SDK should aggregate all values under the same name (run-on-timeframe=["slates", "credits"]) or should take only last seen value (run-on-timeframe="credits"). The former makes more sense from the perspective of users, but in terms of the SDK, it is contradicting the app metadata as the metadata clearly says the value must be a string, not a list of strings. Allowing this behavior and implementing handling of such cases will pose a significant work on the development of the SDK. On the other hand, with the latter behavior, it's not so user-friendly as the former, but users still can achieve their goal by calling the app twice with different parameter values (though it can be fairly inefficient and time-consuming, depend on the internal implementation of the app).

It has to collect both values, otherwise there is no point in doing ?run-on-timeframe=slate&run-on-timeframe=credits. And this should not be a problem for the SDK since all it really needs to do is collect the values and check whether they are in the choices.

Personally I prefer

https://someapp/?run-on-timeframe=slate,credits

I do see there is an issue with giving a list of strings rather than just a single string. We could do this:

"parameters": 
 {
   "name": "run-on-timeframe",
   "type": "string",
   "choices": ["slate", "credits"],
   "multiple-select-allowed": "true", 
   "default": "slate",
   "description": "make app to run only on time frames previously annotated as `slate` or `credits`."
 }

Where the multiple-select-allowed set to true allows you to hand in multiple string values.

keighrim commented 3 years ago

Some answers I can think of now.


Also, here are we assuming that the options for property keys and values are fixed? If a new tool is added that produces a new type for frameType, will downstream apps need to be updated to allow that frame type as a parameter?

I think that's true if the parameter specification of the downstream app uses choices field. One can define a parameter without more-constraining choices but loosely specify possible values in the human readable text (in description field).

Is the objection against not having the frameType=text part in the input?

Yes. The input object is under-specified in that the app in fact wants certain kinds of TimeFrame, but any.

"multiple-select-allowed": "true",

I like this idea. With recent conversation with GBH, we might want to add a parameter to pick labels of interest to the spacy NER app. We can either go with whitelisting or blacklisting, but at the end of the day it can look something like this;

"parameters": 
 {
   "name": "discard-ne-label",  # going with a blacklist
   "type": "string",
   "choices": ["person", "organization", "date", "ord", ... ],
   "multiple-select-allowed": "true", 
   "description": "make app to ignore certain NE labels in the output MMIF"
 }

And finally on the URL form for multi choice params, I also prefer a succinct form

https://someapp/?run-on-timeframe=slate,credits

But, I also believe going with repeating the key with each value is a safer way

https://someapp/?run-on-timeframe=slate&run-on-timeframe=credits

Simply because in the succinct form, we have to worry about parameter values that (possibly) include literal commas , in them, even though that's not so likely to happen.

marcverhagen commented 3 years ago

Also, here are we assuming that the options for property keys and values are fixed? If a new tool is added that produces a new type for frameType, will downstream apps need to be updated to allow that frame type as a parameter?

I think that's true if the parameter specification of the downstream app uses choices field. One can define a parameter without more-constraining choices but loosely specify possible values in the human readable text (in description field).

I think so too. If some app specifies that it uses time frames with frameType=slate then results of an upstream app that generates frameType=credits will not be included. Which is how it should be. At some time I was tempted to propose a choice all, but that is dangerous since you end up processing all time frames including duplicates.

keighrim commented 3 years ago

If you don't use choice at all, it'd be equivalent to all in theory. However, one can hide some code in the actual implementation that's being picky about types of frames. (Kindly letting the user know about it might be a good idea, in such a case)

"input": [
  { "@type": "TimeFrame", 
    "required": false
  }
],
"parameters": 
  {
    "name": "run-on-TimeFrame",
    "type": "boolean",
    "description": "Make app to use existing https://.../TimeFrame annotations. Note that app only knows how to deal with 'slate' and 'credit', so only timeframes types as one of those will be processed. "
  }
...

And the implementation (simplified);

class AnOCRApp(ClamsApp):
  def _appmetadata(self):
    metadata = AppMetadata(...)
    metadata.add_input(at_type=AnnotationTypes.TimeFrame, required=False)
    ... # and more as needed
    return metadata

  def _annotate(self, in_mmif, use_existing_timeframe: bool = False):
    ...
    if not use_existing_timeframe: 
      total_frames = self.get_video_length(mmif.get_document(DocumentTypes.VideoDocument)
      frames_to_process = list(range(total_frames))
    else:
      frames_to_process = []
      for frame_type in ['slate', 'credits']:
        for time_frame in mmif.get_annotations(AnnotationTypes.TimeFrame, frameType=frame_type):
          frames_to_process.extend = list(range(time_frame.start, time_frame.end))
    text_boxes = self.run_ocr(frames_to_process)
    ... 
keighrim commented 1 year ago

I just finished reading this thread from the top here (which was fun, to see how old discussions are later played out and implemented), and I think all the points in the thread are now resolved. Closing as completed.