Skip to content

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented May 9, 2020

This PR proposes to add a Julia application runner for Dash.jl, which will enable the contribution of integration tests for new and existing features for this implementation of the framework.

Here's a simple "smoke" test, replicating the same basic application currently in the R repository:

app = ''' 
using Dash
app = dash("Test app", external_stylesheets = ["https://codepen.io/chriddyp/pen/bWLwgP.css"])

app.layout = html_div() do
 html_div("Hello Dash.jl testing", id="container")
end

run_server(app, "127.0.0.1", 8050)
'''

def test_jstr001_jl_with_string(dashjl):
    dashjl.start_server(app)
    dashjl.wait_for_text_to_equal(
        "#container", "Hello Dash.jl testing", timeout=1
    ) 

The test passes and works within a development branch of Dash.jl.

@waralex @alexcjohnson

self.proc = None

# pylint: disable=arguments-differ
def start(self, app, start_timeout=4, cwd=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The start_timeout needs to be slightly longer than that used for R, since Julia takes a little longer to launch. This should be adequate, but we could always increase to 5 seconds if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather not have to think about this timeout causing problems, especially as we never know what hardware we're on for CI and whether we have its full attention. If 4 "should be adequate" I'd probably give it 5 or 6 :)

Copy link
Contributor Author

@rpkyle rpkyle May 13, 2020

Choose a reason for hiding this comment

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

I was being incredibly optimistic -- 5 or 6 seconds is sufficient for a local run, but within CircleCI it can take 10-12 seconds for Julia to start, and another 5-7 seconds for the HTTP.jl server to respond. I've set the timeouts to 30 seconds, since this seems to provide for consistently passing tests.

We could use 25 seconds, but there are occasional failures. I've tried to add code during the unit test run that triggers the precompilation there rather than within the integration test cycle, but it doesn't really help all that much. I'm not sure why it's slower within CircleCI, for now the conservative defaults will get the tests running.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oof. We don't need to address this now, this is fine for a POC and for running a few tests. But if we get to the point of running lots of integration tests we'll want to see if there's anything we can do to improve the startup time. Either speeding Julia startup in CI, or maybe there's a way we can start one Julia session and reuse it for multiple tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson Yeah, I'd like to see HTTP.jl start up a bit faster -- Julia is often faster than R or Python once the relevant code is loaded, but it does take more time to get going, and some of that is probably unavoidable. But 20-30 seconds for each integration test is going to eventually be problematic. I've wondered why we launch R repeatedly for integration testing, then kill the process.

Is there a reason we don't launch a single process, and then send an interrupt to the Dash server? Even if this itself returns an error, I feel that we should be able to muffle it so the chain of integration tests continues. That would have benefits for all backends, rather than just Julia, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it can be as simple as interrupting the server, though this is what we do for the threaded runner in Python by adding an extra "stop" route. We'll also want to ensure that the environment gets cleaned out so tests don't depend on each other. So I think the solution is going to be different in each language, but anyway it doesn't seem like this overhead is that bothersome in R, whereas in Julia it's a much higher priority.

@alexcjohnson
Copy link
Collaborator

Great that this has such an easy analog to RRunner! Can we 🌴 it? Have RRunner.start and JuliaRunner.start both call out to ProcessRunner._start_command or something? Looks like that would just need self.__class__.__name__ for the log messages and a command template string, then they're the same function.

@alexcjohnson
Copy link
Collaborator

Not new here, but are we cleaning up the temp dirs we create for RRunner (and now JuliaRunner) tests? If we are I don't see it. Seems like we should be able to do that at the end of ProcessRunner.stop - ie if self.tmp_app_path exists at that point, shutil.rmtree it?

@rpkyle rpkyle force-pushed the add-julia-runner branch from 8a2229d to 63353aa Compare May 11, 2020 19:15
@rpkyle
Copy link
Contributor Author

rpkyle commented May 12, 2020

Great that this has such an easy analog to RRunner! Can we 🌴 it? Have RRunner.start and JuliaRunner.start both call out to ProcessRunner._start_command or something? Looks like that would just need self.__class__.__name__ for the log messages and a command template string, then they're the same function.

Yes! Great idea. I've added an issue for that here so we can address this in a subsequent PR, as we discussed offline: #1242.

@rpkyle
Copy link
Contributor Author

rpkyle commented May 13, 2020

Not new here, but are we cleaning up the temp dirs we create for RRunner (and now JuliaRunner) tests? If we are I don't see it. Seems like we should be able to do that at the end of ProcessRunner.stop - ie if self.tmp_app_path exists at that point, shutil.rmtree it?

@alexcjohnson fixed in 62600db, log message added when in debug mode in 46a0cb8:

INFO     dash.testing.application_runners:application_runners.py:84 killing the app runner
INFO     dash.testing.application_runners:application_runners.py:222 proc.terminate with pid 911
DEBUG    dash.testing.application_runners:application_runners.py:226 removing temporary app path /tmp/61b146c3774547b296b264ee0d46e1a8
INFO     dash.testing.application_runners:application_runners.py:244 process stop completes!
INFO     dash.testing.application_runners:application_runners.py:90 __exit__ complete

# try copying all valid sub folders (i.e. assets) in cwd to tmp
# note that the R assets folder name can be any valid folder name
assets = [
os.path.join(cwd, _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(this applies to R as well) does this whole asset-copying business belong in the if cwd block above? If we get here with no cwd it will be None, and interestingly os.listdir(None) succeeds (using the current dir I guess) but os.path.join(None, name) fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 4c4d089

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

LGTM! Just one minor comment that could be deferred to #1242 if you prefer.
💃

@rpkyle rpkyle merged commit ebac7d4 into dev May 19, 2020
@rpkyle rpkyle deleted the add-julia-runner branch May 19, 2020 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants