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

Kwargs to uvicorn run #780

Merged
merged 10 commits into from
Oct 26, 2023
Merged

Kwargs to uvicorn run #780

merged 10 commits into from
Oct 26, 2023

Conversation

gshotwell
Copy link
Contributor

This PR does three things:

  • Add reload-excludes argument to shiny run, for completeness with reload-includes
  • Fixes #765
  • Pass kwargs to uvicorn.run()

I have tested the following cases locally, but we decided that it wasn't worth adding automated tests:

  • Adding an reload-dir causes the app to reload when changes occur in that dir's files
  • Setting shiny run --reload --reload-includes="*.py" app.py causes app to only reload when .py files change
  • shiny run --reload --reload-includes="*.py" --reload-excludes="test.py" app.py causes the app to reload when app.py is changed, but not when test.py is changed.

I couldn't figure out a way to pass **kwargs to the command line, but I still think it's a useful safety valve to include.

shiny/_main.py Outdated Show resolved Hide resolved
shiny/_main.py Outdated Show resolved Hide resolved
shiny/_main.py Show resolved Hide resolved
shiny/_main.py Outdated Show resolved Hide resolved
shiny/_main.py Outdated Show resolved Hide resolved
shiny/_main.py Show resolved Hide resolved
shiny/_main.py Outdated Show resolved Hide resolved
Gordon Shotwell and others added 3 commits October 25, 2023 10:22
Co-authored-by: Barret Schloerke <barret@posit.co>
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

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

LGTM! (pending suggestions and changelog adjustments)
(Please squash merge when checks pass 🥳 )

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -59,6 +60,9 @@ Methods still under consideration in `shiny.experimental.ui`:
* `card(wrapper=)`, `card_body()`, `card_image()`, `card_header()`


### Bug fixes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this section be combined with ### Bug fixes below?

Comment on lines +292 to 293
if app_dir and app_dir not in reload_dirs:
reload_dirs.append(app_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

app_dir is added to reload_dirs above iff reload_dirs is its default value and app_dir is supplied.
So, we should remove this enhancement.

Suggested change
if app_dir and app_dir not in reload_dirs:
reload_dirs.append(app_dir)

Gordon Shotwell and others added 2 commits October 26, 2023 10:18
Co-authored-by: Barret Schloerke <barret@posit.co>
@gshotwell gshotwell merged commit 30ef70e into main Oct 26, 2023
24 checks passed
@gshotwell gshotwell deleted the kwargs-to-uvicorn-run branch October 26, 2023 17:12
@dbkegley
Copy link

I couldn't figure out a way to pass **kwargs to the command line, but I still think it's a useful safety valve to include.

Coming in just a little bit late on this one, but I was just looking for a way to pass --root-path to uvicorn and I saw that kwargs were added to uvicorn.run().

I'm exposing my app behind a reverse proxy and request.url_for is returning the incorrect path.

encode/starlette#604

Is there a workaround for passing in kwargs to shiny run or should I look at creating a uvicorn config file?

schloerke added a commit that referenced this pull request Nov 2, 2023
* main: (56 commits)
  Add actions step for nightly reporting -> Testrail (#774)
  Remove unused entry in manifest
  Bump version to 0.6.0.9000
  Use latest htmltools
  Bump version to v0.6.0; Rearrange news to have API changes last
  Tweaks from testing
  Fix default width/height with implicit plot output (#792)
  Update deps (#794)
  Remove deprecated 'name' parameter from `Outputs` (#791)
  api(ui): Drop `toggle_` methods. Consolidate update accordion methods. Stronger typing for `layout_sidebar(sidebar)` and `page_sidebar(sidebar)` (#788)
  bug(sidebar): Revert sidebar icon back to chevron (#789)
  For `input_action_button`, default to having whitespace around button (#758)
  Remove output from template app (#775)
  Add output_args and suspend_display decorators (#786)
  Update value_box; Update to bootstrap 5.3; Update htmldeps (#772)
  tests: add sidebar test (#787)
  Seaborn plots should fill their output_plot (#785)
  Kwargs to uvicorn run (#780)
  Add width and height arguments to `@render.plot` (#783)
  gitgnore dist/
  ...
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.

Option --reload_dirs have no effect
3 participants