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

Move Script Cache Clearing to Dagger #5436

Merged

Conversation

nbauernfeind
Copy link
Member

@nbauernfeind nbauernfeind commented Apr 30, 2024

The QueryCompiler change somehow now requires that the directory is cleared before the sub-directory is created. The simplest solution I could find defers the parent directory clearing to just prior to the session construction.

Fixes #5426

@nbauernfeind nbauernfeind added core Core development tasks python release blocker A bug/behavior that puts is below the "good enough" threshold to release. python-server-side labels Apr 30, 2024
@nbauernfeind nbauernfeind added this to the 2. April 2024 milestone Apr 30, 2024
@nbauernfeind nbauernfeind self-assigned this Apr 30, 2024
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

This seems "OK". I assume that in a multi-ScriptSession world, though, we would want to do this its own module that we force to run before we bind a session?

@niloc132
Copy link
Member

This seems "OK". I assume that in a multi-ScriptSession world, though, we would want to do this its own module that we force to run before we bind a session?

I believe we would first need to stop conflating "class created for script" (specifically for groovy) and "class created for formulas" - the QueryCompiler also writes to this dir, which is I believe how this ended up mattering.

I still don't understand why moving this call slightly earlier makes a difference though, since DeephavenApiServer.run() is already called by EmbeddedServer.start()?

@niloc132
Copy link
Member

I believe we would first need to stop conflating "class created for script" (specifically for groovy) and "class created for formulas" - the QueryCompiler also writes to this dir, which is I believe how this ended up mattering.

Reframing this slightly, I think we probably want one dir per startup (since the dir is for the QueryCompiler), and multiple groovy sessions either should stop using that directory, or start using specific packages so they don't stomp on each other. Python doesn't use this dir directly at all.

Note that we still have the problem of starting two servers even in "different directories" since we default to a user directory rather than invocation specific - for example ~/.cache/deephaven/script-session-classes/<uuid> on linux, with all other uuids deleted on startup (later follow up probably should loop in Devin/Bender).

@nbauernfeind nbauernfeind merged commit 67057c5 into deephaven:main Apr 30, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core Core development tasks NoDocumentationNeeded python python-server-side release blocker A bug/behavior that puts is below the "good enough" threshold to release. ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update expressions run in embedded python server error
3 participants