flywheel-io / gears

Flywheel Gear Examples and Documentation
MIT License
6 stars 3 forks source link

Context input type #26

Closed kofalt closed 6 years ago

kofalt commented 6 years ago

Frequently, there is need for configuration that is not really specific to the gear, and more specific to the organizational / logistical environment. Examples: matlab licensing key, project-specific override value.

Context values:

  1. Have no type
  2. Are not guaranteed to be present
  3. Are provided in the gear's config.json if found

Provisioning and lookup of context values are out of scope for the gear spec. An implementation could decide to provide context values any way it wants, or allow a user to modify it per-job.

For our implementation, the lookup process would walk up the hierarchy, checking container metadata for context.[VALUE], providing the first match if any and null if not found.

An example manifest excerpt:

"inputs": {

    // Normal file
    "dicom": {
        "base": "file",
        "type": { "enum": [ "dicom" ] },
        "description": "Any dicom file."
    },

    // Contextual key-value
    "matlab_license_code": {
        "base": "context"
    }
}

Results in config.json:

{
    "config": {
    },
    "inputs" : {
        "dicom" : {
            "base" : "file",
            "hierarchy" : {
                "type" : "acquisition",
                "id" : "5988d38b3b49ee001bde0853"
            },
            //...
        },

        // Context was found
        "matlab_license_code": {
            "base" : "context",
            "found": true,
            "value" : "aex"
        }
    }
}
travisr commented 6 years ago

How could we use the same concept for config, but provide a default value if not found?

kofalt commented 6 years ago

I suppose we could let the gear set a default value if the context is not found.

travisr commented 6 years ago

Let's make sure we cover that. The idea is to enable a project, group, or site, to set their own defaults for use with a given gear. Ideally, we would still be able to edit them in the normal way when manually running a gear.

nagem commented 6 years ago

We should make sure any workflows we set up here aren't counter intuitive when gear config is added to rules: https://github.com/scitran/core/pull/961

nagem commented 6 years ago

This would make context a "reserved" key on info then, correct? Or would context be a top level key?

nagem commented 6 years ago

Do we expect anything under context to be sensitive information or can anyone with permissions to the project view this information? Who is allowed to edit it? I'm wondering if it being stored directly on the container is the best option (would need to be filtered out on GETs) or if a using a separate collection with a foreign key to the container is a better solution.

travisr commented 6 years ago

would be useful to have this mechanism be rich enough to save all default values for a given gear (possibly by having a "save as default" button on the config page of an analysis or on the gear run dialog. This would provide a project wide default for batch or manually run gears. It could also simplify entering the config attached to gear rules. The context variable would have to be nested to handle a set of values for each gear.

kofalt commented 6 years ago

We should make sure any workflows we set up here aren't counter intuitive when gear config is added to rules: scitran/core#961

I would expect context to be provided the same way by a gear launched manually or automatically, so I don't see a conflict on that. It would have to be implemented in the rule launch logic.

This would make context a "reserved" key on info then, correct?

Correct, for some value of "reserved". Semantically meaningful, yes.

Do we expect anything under context to be sensitive information or can anyone with permissions to the project view this information? Who is allowed to edit it?

No, yes, and everyone with read access respectively.

would be useful to have this mechanism be rich enough to save all default values for a given gear (possibly by having a "save as default" button on the config page of an analysis or on the gear run dialog. This would provide a project wide default for batch or manually run gears. It could also simplify entering the config attached to gear rules. The context variable would have to be nested to handle a set of values for each gear.

I don't think this proposal is well set out to handle that, that sounds more like gear config templates (which I agree we should chase). That probably won't show up in the gear spec though.

travisr commented 6 years ago

While I love the idea of defaults for gear config, I will back off for now. Let's just keep the scope of this to a basic set of environment variables under the context (or whatever we decide to call it)

ryansanford commented 6 years ago

I do like any time we can eliminate the need to use the FW SDK within the gear. The utility with this proposal is impressive.

Is there danger in that utility creating a blurred line between config and input context? For example, as a gear author, why should I not be tempted to replicate all my gear config parameters as context objects as well to achieve the utility of project-level-gear-config?

I feel like we corrected a similar pattern where we incentivized gear authors to implement config default logic, due to the FW implementation not initially providing those defaults for gear rule jobs.

kofalt commented 6 years ago

Is there danger in that utility creating a blurred line between config and input context?

I don't think so, but part of the intention is that it's pretty freeform. It's up to the implementation to either incentivize or deincentivize that as desired.

kofalt commented 6 years ago

There's enough buyin from inside & outside the thread that I'm comfortable going forward. Anyone curious is welcome to comment on the upcoming PR.