-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Dbg r reload #983
Changes from 5 commits
e32795e
a6af1c8
990f424
917b2af
66fcdd7
8d87f6d
445ba1e
9077f56
09e229a
fae4efb
70a4552
3bf3b3b
a88542f
74be160
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,9 @@ | |
import uuid | ||
import shlex | ||
import threading | ||
import shutil | ||
import subprocess | ||
import distutils | ||
import logging | ||
import inspect | ||
|
||
|
@@ -210,9 +212,7 @@ def start( | |
args, stdout=subprocess.PIPE, stderr=subprocess.PIPE | ||
) | ||
# wait until server is able to answer http request | ||
wait.until( | ||
lambda: self.accessible(self.url), timeout=start_timeout | ||
) | ||
wait.until(lambda: self.accessible(self.url), timeout=start_timeout) | ||
|
||
except (OSError, ValueError): | ||
logger.exception("process server has encountered an error") | ||
|
@@ -255,19 +255,18 @@ def start(self, app, start_timeout=2, cwd=None): | |
"""Start the server with subprocess and Rscript.""" | ||
|
||
# app is a R string chunk | ||
if (os.path.isfile(app) and os.path.exists(app)): | ||
if os.path.isfile(app) and os.path.exists(app): | ||
# app is already a file in a dir - use that as cwd | ||
if not cwd: | ||
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)) | ||
) | ||
tmp = os.path.join( | ||
"/tmp" if not self.is_windows else os.getenv("TEMP"), | ||
uuid.uuid4().hex, | ||
) | ||
path = os.path.join(tmp, "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") | ||
|
@@ -283,11 +282,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( | ||
|
@@ -297,9 +296,28 @@ 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, _)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🐄 why There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
1-char name is anti-pylint There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(tmp, 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, tmp) | ||
distutils.dir_util.copy_tree(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) | ||
|
@@ -309,13 +327,13 @@ def start(self, app, start_timeout=2, cwd=None): | |
args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd | ||
) | ||
# wait until server is able to answer http request | ||
wait.until( | ||
lambda: self.accessible(self.url), timeout=start_timeout | ||
) | ||
wait.until(lambda: self.accessible(self.url), timeout=start_timeout) | ||
|
||
except (OSError, ValueError): | ||
logger.exception("process server has encountered an error") | ||
self.started = False | ||
return | ||
|
||
self.started = True | ||
|
||
return app | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rpkyle in case you need to get the tmp path, |
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.
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