Skip to content
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

Simplify ScriptSession dependencies #4190

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Conversation

niloc132
Copy link
Member

This removes the only python init script, and removes an unused groovy init script. The config variables are left in place for now, though they will default to blank.

One slightly unexpected change that this results in is that the jpy, sys, and os modules are no longer imported by default into a python script session - this probably should be considered a feature, but does need to be documented.

In a future patch I'll propose unifying how init scripts are run, so that there is no potential loss in functionality. If reviewers would prefer, I can just go ahead and add those changes here too, though my aim was to let this be a smaller step by itself.

devinrsmith
devinrsmith previously approved these changes Jul 17, 2023
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Definitely in favor of these types of changes.

jmao-denver
jmao-denver previously approved these changes Jul 18, 2023
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM


def init_py():
"""Finishes starting Python to be usable from inside of a Java process. Not intended to be called in cases
where there process was start as Python, and Java was started from inside Python.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
where there process was start as Python, and Java was started from inside Python.
where there process was started as Python, and Java was started from inside Python.

Copy link
Member Author

Choose a reason for hiding this comment

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

also fixed there->the

Comment on lines +85 to +86
# If you want jpy to tell you about all that it is doing, change this
# jpy.diag.flags = jpy.diag.F_ALL
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an ENV controlled option to help us debug users?

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems like a decent idea, but in a different PR (i'm just moving the contents of this file so we have one fewer way to screw up startup).

rcaudy
rcaudy previously approved these changes Jul 20, 2023
engine/table/build.gradle Outdated Show resolved Hide resolved
@niloc132 niloc132 merged commit bf579e0 into deephaven:main Sep 20, 2023
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2023
@deephaven-internal
Copy link
Contributor

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3211
Reference: https://github.com/deephaven/deephaven.io/issues/3210

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants