Initial content

[athornton]

No description provided.

[athornton]

I think we need both mode and document-type because our substitution strategy might vary by document type, even if we're just doing a render. In particular, rather than doing the templating ourselves for notebook types, we probably want to leverage Times Square, but it sounded like Jonathan was leery of the scope creep if we turned that into a generic templating engine for all sorts of documents.

Having document-type also lets us do better validation on the returned document: that is, for a notebook, we can:

1: make sure that the base64-encoded payload unpacks to a byte array,
2: ensure that the byte array decodes as the UTF-8 representation of a Unicode string,
3: that string is loadable as JSON,
4: that JSON conforms to https://ipython.org/ipython-doc/3/notebook/nbformat.html

[stvoutsin]

Overall I think the core spec looks solid, just a few thoughts/questions:

• I generally find descriptive names slightly better instead of Dixon and Keene, but maybe that's just me
• Is it worth expanding how third-party support would work, how are plugins discovered at runtime?
• What type flows between the parsers in the chain? If a parser calls Keene in open-notebook mode and gets back a URL, what does the next parser receive? Should this be clarified?
• How do the Keene error codes propagate to the user? What would a user see from a 500 error in Keene?
• Is it worth clarifying the choice of dict[str, byte[]] over dict[str, str]?
• For cases where file exists should we return 409 instead of 403?
• The current ensure_running_lab hook redirects users to the JupyterHub spawner form so they can choose their image and size. As I understand it this is changing and we now choose the default image/size for them. Are we 100% sure that is what users want, or is it possible they'd prefer to be able to choose a specific image?
• If this is delegating the templating to Times Square, will it be able to handle arbitrary Github repos? Is that functionality already there or planned to be there in Times Square? Is Times Square available on all envs where Ghostwriter will be?
• What is the "consumer" of the render mode for Keene?
• Regarding the Dixon/Keene split, if they end-up with in-memory Python communication, i.e. two modules I'd treat them as the same service which is close to the architecture you already have?

[athornton]

• I'm not necessarily opposed, but "Presenter" and "Renderer" are really boring. Counterpoint: Gafaelfawr.
• It would be, but I think I'm going to have to put down a test implementation before I have much of a feel for how third-parties really would use this, given that I don't have a super-clear idea of how I'm going to put this together.
• A parser gets a Keene response, which will be a JSON document, which will be used as an input for the next rewrite() in the chain. But also, you're right: part of "I don't think I'm going to know until I implement something" is why I say "The typing for rewrite() is not yet fully determined"; the Request will be the incoming request as modified by the information from the parser it just traversed.
• I think if Keene fails, then probably what the user gets is an error page rendered by Dixon? I need to think about that.
• On further thought, I think I want to use dict[str,str] with an escape hatch, like, if I really need binary (which I may never, and which would be a very rare case, I think; overwhelmingly, these are UTF-8 strings), then if the value begins with __base64: then the rest of the string is interpreted as a base64-encoding of the decoded binary object. That does mean if you want a string that starts with __base64: as your actual payload, you'll need to encode it again.
• Yes, 409 Conflict is more appropriate.
• I am not sure what users want. I'm happy to implement it either way. This is analogous to the same problem for Times-Square so we should maybe figure out how to specify policies for both at once.
• I don't think we want to handle truly arbitrary GitHub repos; at the most we want to allow it for orgs we control (lsst, lsst-dm, lsst-ts to start with, I think?). Jonathan seemed leery of making Times Square a very generic service, so I'm happy to work with his constraints; it feels silly at some level to have two template-rendering engines, though. There's no reason we cannot declare that Times Square is a dependency of Ghostwriter.
• Dixon consumes the output of the Render mode, and it will return a redirect that ultimately opens the user's running showing the rendered notebook.
• Yeah, if they're modules within the same service, this ends up a lot like /ghostwriter and /ghostwriter/rewrite. I suspect that's the right way to do it

[athornton]

Updated with 409 for no-overwrite file conflict and str parameter values (with note on base64 if we actually need arbitrary byte sequences).