-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add console scripts to start Deephaven from the command line #5275
Conversation
if args.key is None: | ||
args.key = secrets.token_urlsafe(32) | ||
|
||
jvm_args = [f"-Dauthentication.psk={args.key}"] | ||
if args.jvm_args is not None: | ||
jvm_args += args.jvm_args.split() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something we've talked about before is creating the config file if it doesn't exist and writing the PSK into it; this gives it some persistence. I think that's how jupyter does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jupyter doesn't seem to persist the token. If I run jupyter lab
twice in a row, I get new tokens. I think that makes sense, it shouldn't persist unless you specify it.
That being said I'm not against it and Jupyter certainly doesn't have to be precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay; do you know if a fresh jupyter lab writes down any configuration files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it writes a couple of files it looks like:
/home/bender/.jupyter/lab/workspaces/default-37a8.jupyterlab-workspace
: Config for the users workspace, such as layout of the panels/home/bender/.local/share/jupyter/runtime/jupyter_cookie_secret
: I'm not 100% sure what this cookie is for; it's not the token used for auth (that regenerates each time) and it just regenerates this if you delete the file. Searching for details on what that cookie is used for isn't turning up much (I checked the jupyter and jupyterlab orgs on GitHub). There's some mention of it in forums indicating it'll fail if permissions don't allow reading the file, but it will regenerate if the file doesn't exist.
So again, we can write a config, I don't mind writing the token to a file for persistence, but that doesn't seem to be what Jupyter is storing (and I'm actually not sure what the secret cookie they're storing is anyway. Some sort of base64 string, but it doesn't seem to be stored in the browser as a cookie, doesn't seem to have a reference in any of the accessible Jupyter code that I've searched ... I don't know what it is?
Note from community user:
We might want to consider inheriting the EXTRA_CLASSPATH environment variable (and or START_OPTS / JAVA_OPTS)? |
- Looking good!
- Move _start to after main
- click uses it's own kind of argument parsing which is more optimal
65983a3
to
573a001
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got some suggestions that expose more of the configuration properties to EmbeddedServer, PR mofojed#8.
The one other thing that I think will be important is moving the console entry point out of __init__.py
. I think users who import deephaven_server should not need to source the click / cli logic.
Plumb ServerConfig / AuthenticationRequestHandler through EmbeddedServer
sys.exit(0) | ||
|
||
signal.signal(signal.SIGINT, signal_handler) | ||
signal.pause() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great.
- I would add an optional argument to
server
that takes a path to a script file to run before getting into the wait loop. Basically a way to populate DH with a quick command. - I would add a new
run
command to run a script. Basically the same asserver
without the auth, browser, and wait. Batch mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of those options sound great. Mind if it's a future PR?:
- That's kind of like application mode, would we want to specify the "app" to start with? I believe the only way to do that now is by setting the app.d directory, so we'd want to think how we have this play out/work with application mode.
- Similar to question 1, do we still have the application mode scripts run or just the file you specified? What config do we want to override in that case?
In any case with the console scripts added, we can add new commands/options fairly easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it soon.
if anonymous: | ||
jvm_args += [f"-DAuthHandlers=io.deephaven.auth.AnonymousAuthenticationHandler"] | ||
elif key is not None: | ||
jvm_args += [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this prevents a user from specifying mTLS/etc as their preferred option, without forcing that they also have PSK (or anon) enabled as well... and it will override their AuthHandlers
value and wipe out their own config.
Unsure if we want to get into the business of parsing AuthHandlers or other config vars here... if we want to go after this it might be time to tighten up our config language/control, or just be prepared for breaking changes.
Or put dh_args back or something, and require that it be used instead of system props for dh configs? and have some kind of schema here for "this is actually a ,
-delimited list of strings"?
I kind of hate all of the choices... I do have a draft proposal for "we should use TOML", but I'm not sure now's the time to pick that up in earnest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, these shouldn't be mutually exclusive - one day we'll have authorization hooks that are easy to use, and we'll want to allow both (anon might be simple, psk might be admin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping we can discourage users from setting --anonymous
or --psk
wrt a deephaven server start command (in my helper PR I originally removed them). When they are not provided, the server will retain it's normal defaults or source from the users configuration file. I also suggested that deephaven set-config ...
might be a reasonable command that allows us to explicitly present a way to do "proper" configuration from the CLI (ie, writing down into a configuration file w/ proper safeguards).
There was a similar concern I had for jsPlugins configuration, essentially relying on a prefix call:
class PropertyFile {
...
/**
* Collect all of the properties in this property file that begin with a given prefix.
*
* @return a new Properties object containing the selected properties, with the prefix removed.
*/
public Properties getProperties(String prefix) {
to allow independent configuration. ie
deephaven.jsPlugins.foo=/path/to/plugin-1
deephaven.jsPlugins.bar=/path/to/plugin-2
as opposed to needing to do
deephaven.jsPlugins=/path/to/plugin-1,/path/to/plugin-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want or need deephaven server
command line options to present the full universe of configuration possibilities to users; in my mind, these options may be better thought of as --only-psk=<key>
, --only-anonymous
, etc. I am leaning that maybe we should remove them from deephaven server
altogether, I don't think it would be a big loss. (I could make similar argument for --host
and --port
, but I think that's a slightly harder sell.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be alright with getting rid of the anonymous/psk options now that there's the targetUrls
option, as those options conflicted with existing config. Leaving jvm_args
seems alright, it adds to the args in your existing config. host
/port
seem like they'd be nice and would just override any config you have which makes sense IMO.
- Add missing pydocs and javadocs
target_url_or_default = s.server_config.target_url_or_default | ||
authentication_type = None | ||
authentication_url = target_url_or_default | ||
for authentication_handler in s.authentication_handlers: | ||
authentication_urls = authentication_handler.urls(target_url_or_default) | ||
if authentication_urls: | ||
authentication_type = authentication_handler.auth_type | ||
authentication_url = authentication_urls[0] | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the route I originally took when creating this feature; but it became clear that I was trying to service this one specific use case as opposed to being more library-like and presenting the components themselves.
I could see an argument for moving this logic into the python side Server
; I don't necessarily think the java side EmbeddedServer
is the correct place. Once you elevate it to Server
though, it becomes a public API and something we need to treat as a library feature more generally.
""" | ||
return self.j_authentication_handler.getAuthType() | ||
|
||
def urls(self, target_url: str) -> List[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devinrsmith Also this gives the URL to the IDE - just wondering, if we want URLs to the iframe endpoints as well, that could be useful for deephaven-ipywidgets (and others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API doesn't assume much about target_url
; in the case of anonymous, it just echos it back, and in the case of PSK, just appends ?psk=<key>
. It doesn't know anything about IDE nor iframe, etc.
By default, target_url = "http://localhost:10000"
. As part of CLI, you could append /ide/
to be a bit more explicit, or invoke the logic multiple times to get iframe or other links.
It would be nice to have a more proper API around web apps installed as opposed to doing:
for (String appRoot : List.of("/ide/", "/iframe/table/", "/iframe/chart/", "/iframe/widget/")) {
Until then though, I think you'll need to do something similar in CLI if you want to.
target_url_or_default = s.server_config.target_url_or_default | ||
authentication_type = None | ||
authentication_url = target_url_or_default | ||
for authentication_handler in s.authentication_handlers: | ||
authentication_urls = authentication_handler.urls(target_url_or_default) | ||
if authentication_urls: | ||
authentication_type = authentication_handler.auth_type | ||
authentication_url = authentication_urls[0] | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that said, this cli is a public API - once we put it out there, we can't break it (without being the kind of jerk that would break any other API).
- Now that the URL is fetched from the server, we don't need to pass in the key to be able to open the browser successfully - Could have multiple different types of authentication, so these options kind of hamstring things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#185 |
deephaven
console script that can launch deephavendeephaven_server
whldeephaven
is the root script, withserver
being the only positional "command" argument and ran by defaultrun
to run a specific file)host
,port
,jvm-args
, andextra-classpath
Examples:
deephaven server
: Starts up Deephaven server on port 8080 with a random key and opens the web UI.deephaven server --port 9999
: Starts up Deephaven on port 9999.deephaven server --jvm-args="-Ddeephaven.dataDir=/tmp/deephaven"
: Starts up Deephaven with/tmp/deephaven
as the data directory.deephaven server --port 9999 --jvm-args="-Ddeephaven.dataDir=/tmp/deephaven-data -Ddeephaven.console.disable=true"
: Starts up Deephaven on port 9999, using/tmp/deephaven
as the data directory and the console disabled.deephaven --help
: Prints help about what commands are availabledeephaven server --help
: Prints help about the server comand