Skip to content

Dbg r reload #983

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

Merged
merged 14 commits into from
Oct 30, 2019
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
All notable changes to `dash` will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## Unreleased
### Fixed
- [#983](https://github.com/plotly/dash/pull/983) Fix the assets loading issues when dashR application runner is handling with an app defined by string chunk.

## [1.5.1] - 2019-10-29
### Fixed
- [#987](https://github.com/plotly/dash/pull/987) Fix cache string handling for component suites with nested folders in their packages.
Expand Down
55 changes: 45 additions & 10 deletions dash/testing/application_runners.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import uuid
import shlex
import threading
import shutil
import subprocess
import logging
import inspect
Expand Down Expand Up @@ -61,6 +62,7 @@ def __init__(self, keep_open, stop_timeout):
self.started = None
self.keep_open = keep_open
self.stop_timeout = stop_timeout
self._tmp_app_path = None

def start(self, *args, **kwargs):
raise NotImplementedError # pragma: no cover
Expand Down Expand Up @@ -103,6 +105,10 @@ def url(self):
def is_windows(self):
return sys.platform == "win32"

@property
def tmp_app_path(self):
return self._tmp_app_path


class ThreadedRunner(BaseDashRunner):
"""Runs a dash application in a thread.
Expand Down Expand Up @@ -261,13 +267,18 @@ def start(self, app, start_timeout=2, cwd=None):
cwd = os.path.dirname(app)
logger.info("RRunner inferred cwd from app path: %s", cwd)
else:
path = (
"/tmp/app_{}.R".format(uuid.uuid4().hex)
if not self.is_windows
else os.path.join(
(os.getenv("TEMP"), "app_{}.R".format(uuid.uuid4().hex))
)
self._tmp_app_path = os.path.join(
"/tmp" if not self.is_windows else os.getenv("TEMP"),
uuid.uuid4().hex,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as @rpkyle mentioned, the hot reloading on R side will actively monitor all the folder under the main app file, in this case, the old approach using /tmp/app_{}.R is not safe

)
try:
os.mkdir(self.tmp_app_path)
except OSError:
logger.exception(
"cannot make temporary folder %s", self.tmp_app_path
)
path = os.path.join(self.tmp_app_path, "app.R")

logger.info("RRunner start => app is R code chunk")
logger.info("make a temporary R file for execution => %s", path)
logger.debug("content of the dashR app")
Expand All @@ -283,11 +294,11 @@ def start(self, app, start_timeout=2, cwd=None):
for entry in inspect.stack():
if "/dash/testing/" not in entry[1].replace("\\", "/"):
cwd = os.path.dirname(os.path.realpath(entry[1]))
logger.warning("get cwd from inspect => %s", cwd)
break
if cwd:
logger.info(
"RRunner inferred cwd from the Python call stack: %s",
cwd
"RRunner inferred cwd from the Python call stack: %s", cwd
)
else:
logger.warning(
Expand All @@ -297,16 +308,40 @@ def start(self, app, start_timeout=2, cwd=None):
"dashr.run_server(app, cwd=os.path.dirname(__file__))"
)

# 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, _)
for _ in os.listdir(cwd)
if not _.startswith("__")
and os.path.isdir(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.

🐄 why _? I prefer to use that only when the var isn't going to be used at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also like to use that in list comprehension especially when it gets more complicated.

  • shorter
  • I think it's easier to read once you used to the convention, and can be more focus on the operation and logic side.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I'd tend to use a 1-character name for that but I see your point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'd tend to use a 1-character name for that but I see your point.

1-char name is anti-pylint

Copy link
Collaborator

Choose a reason for hiding this comment

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

_ isn't a 1-char name? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, it's a valid 1-char name, certified.

]

for asset in assets:
target = os.path.join(
self.tmp_app_path, os.path.basename(asset)
)
if os.path.exists(target):
logger.debug("delete existing target %s", target)
shutil.rmtree(target)
logger.debug("copying %s => %s", asset, self.tmp_app_path)
shutil.copytree(asset, target)
logger.debug("copied with %s", os.listdir(target))

logger.info("Run dashR app with Rscript => %s", app)

args = shlex.split(
"Rscript {}".format(os.path.realpath(app)),
"Rscript -e 'source(\"{}\")'".format(os.path.realpath(app)),
posix=not self.is_windows,
)
logger.debug("start dash process with %s", args)

try:
self.proc = subprocess.Popen(
args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd
args,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
cwd=self.tmp_app_path if self.tmp_app_path else cwd,
)
# wait until server is able to answer http request
wait.until(
Expand Down