Skip to content
This repository has been archived by the owner on Jul 16, 2020. It is now read-only.

Context input type #30

Merged
merged 1 commit into from
Mar 21, 2018
Merged

Context input type #30

merged 1 commit into from
Mar 21, 2018

Conversation

kofalt
Copy link
Member

@kofalt kofalt commented Mar 14, 2018

Provides typeless, no-guarantee contextual key/value pairs on demand.
Also took the opportunity to clean up the generator a bit and add output to validate.py.

Closes #26. Merges after #28, see versioning notes there.

@kofalt kofalt requested review from gsfr, ehlertjd, lmperry and tcbtcb March 14, 2018 20:54
'value': { }, # Context inputs can have any type, or none at all
},
'required': [ 'base', 'found', 'value' ]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better place we can put this spec in the long run? I think it would improve readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm totally open to different approaches. Templating a JSON schema might be its own unreadable mess, but maybe these could eventually become separate JSON files that are loaded & modified?

@@ -100,10 +100,10 @@ Note, the `// comments` shown below are not JSON syntax and cannot be included i
},

"description": "A set of 3D coordinates."
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing comma

// Not guaranteed to be found - the gear should decide if it can continue to run, or exit with an error.
"matlab_license_code": {
"base": "context",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing comma

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually leave those in intentionally, since the comments make it invalid JSON anyway (I have the code block set to js syntax), just so diffs can be cleaner. Maybe that's dumb.

Context inputs are values that are generally provided by the environment, rather than the human or process running the gear. These are generally values that are incidentally required rather than directly a part of the algorithm - for example, a license key.

It is up to the gear executor to decide how (and if) context is provided. In the Flywheel system, the values can be provided by setting a `context.key-name` value on a container's metadata. For example, you could set `context.matlab_license_code: "AEX"` on the project, and then any gear running in that project with a context input called `matlab_license_code` would receive the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we resolve how the full context will be assembled in the flywheel system? (i.e. do we walk up the tree, taking values in the closest container to populate the context?) Maybe this isn't the place to expound on details of the Flywheel system, but since Flywheel was mentioned, can this link to documentation that has more detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and yes once it exists respectively. I do want to keep a clean separation - not much point to a spec if it can't be implemented at least twice - and the FW contexts will be implemented after this merge, so let's update with a link to docs.fw in a future PR.

Copy link
Contributor

@ehlertjd ehlertjd left a comment

Choose a reason for hiding this comment

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

Nothing that should prevent this from merging.
LGTM

@kofalt kofalt merged commit 6acf4d9 into master Mar 21, 2018
@kofalt kofalt deleted the context branch March 21, 2018 21:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants