-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat: Support templates with _template.json
metadata
#1631
Conversation
Used both internally and for external template repos provided via `--github` command line option
* Rename `use_internal_template()` (replaces `use_template_internal()`) * Separate `js_component_questions()` into * `use_internal_package_template()`: questions related to picking an internal package template * `package_template_questions()`: questions related to setting up and copying the package template
Also start marking internal menu choices with a leading underscore so they aren't confused with template choices
type: str = "text", | ||
): | ||
self.text = text | ||
self.type: Literal["action", "info", "warning", "danger", "text"] = "text" |
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.
Why not have the Literal
type in the function signature?
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.
Because the incoming value is from untyped JSON
def use_internal_chat_ai_template( | ||
input: str | None = None, | ||
dest_dir: Optional[Path] = None, | ||
package_name: Optional[str] = None, | ||
): | ||
if input is None: | ||
input = questionary.select( | ||
"Which kind of generative AI template would you like to use?", | ||
choices=[ | ||
Choice(title="By provider...", value="_chat-ai_hello-providers"), | ||
Choice(title="Enterprise providers...", value="_chat-ai_enterprise"), | ||
back_choice, | ||
cancel_choice, | ||
], | ||
style=styles_for_questions, | ||
).ask() |
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.
It doesn't feel great to be "exporting" _templates.json
with no way to support choice groups (i.e., sub-menus).
If there was one _templates.json
per shiny create
, it'd be fairly easy to support choice groups, but with one _templates.json
per app, it's definitely harder.
Would you mind reminding me of the benefits of one _templates.json
per app?
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.
It's so much easier to work with a single json file per template than in one large JSON file. With atomic template metadata, it's much easier to keep track of the metadata for the template. For example, I just moved the chat templates from examples/chat
to shiny/templates/chat
and had to change one string in our code base. Then I renamed shiny/templates/app-templates
to shiny/template/app
and again made one small change.
With the --github
flag, it's easy to do one-level choice groups with the current setup. Imagine we put the chat templates in py-shiny-templates
. Then you can construct a shiny create
command that only lists those chat templates, ignoring everything else in the repo:
shiny create --github posit-dev/py-shiny-templates:chat/hello-providers
We could certainly implement choice groups using an additional metadata file, but organizing templates by directory with atomic template files is pretty powerful. In practice, I think we'd use the GitHub repo path formulation to limit choices for a certain type of template within a template repo.
Overall, this approach feels like a good 80% of what we want without rewriting from scratch, over investing in abstractions, or closing off the opportunity to build on this in the future.
I guess the big thing is that, if we find we have a need we can easily extend the current approach to build submenus. I certainly see the value in having support for submenus, but I don't think the need is strong enough at this moment for it to be needed.
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.
easily extend the current approach to build submenus
If you have something in mind, would you mind writing it up in an issue? It would be really nice if shiny create --github posit-dev/py-shiny-templates
gave you a similar submenu interface (especially if we keep adding templates, it's going to feel overwhelming).
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.
@@ -4,7 +4,7 @@ | |||
"title": "Intermediate dashboard", | |||
"description": "An intermediate dashboard with value boxes, several plots in cards and a sidebar.", | |||
"next_steps": [ | |||
"Run the app with `shiny run app.py`." | |||
"Run the app with `shiny run app.py` from the app directory." |
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.
Sorry, the suggestion I made for this was intended for all the templates that have this next step (i.e., let's do them all or none)
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.
Ok. Honestly, I don't love this. I think next_steps
is needed generally, but this advice is specific to our internal templates where we can write more direct instructions using questionary.
e.g. I'd rather we offer to open the app.py
in VS Code or Positron, or not in an editor, at which point our advice for running the app would change.
At the very least we could actually include the app directory in the instructions.
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.
Actually, only the basic-app
template needs this instruction, otherwise we already have instructions to move into the app directory. (88d4c9f)
❯ shiny create
? Which template would you like to use? Intermediate dashboard
… Creating Intermediate dashboard Shiny app...
? Enter destination directory: ./04-dashboard-tips
? Would you like to use Shiny Express? Yes
✓ Created Shiny app at 04-dashboard-tips
→ Next steps:
- Install required dependencies:
cd 04-dashboard-tips
pip install -r requirements.txt
- Open and edit the app file: 04-dashboard-tips/app.py
- Run the app with `shiny run app.py`.
ℹ Just getting started with Shiny?
→ Learn more at https://shiny.posit.co/py/docs/user-interfaces.html
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.
FYI we keep the full path in
- Open and edit the app file: 04-dashboard-tips/app.py
because VS Code makes this path clickable. If we changed it to app.py
we'd break click-to-open. But I think it's reasonable to expect the working directory change to carry over to the shiny run
command.
Just remembered that I want to change the {
"type": "app",
"id": "basic-sidebar",
"title": "Sidebar layout",
"description": "An app with inputs in a sidebar and a plot in the main area."
} |
5b4d619
to
bda4010
Compare
* main: CI(deploy): Add more installation configs to surface failure cause (posit-dev#1658) `Chat.messages()` no longer trims messages by default (posit-dev#1657) tests(controllers): Split _controls.py into separate files (posit-dev#1652) Chat tweaks (posit-dev#1607) docs: Add modal_show/remove examples (posit-dev#1628) Delay sending of chat UI messages until reactive graph is flushed (posit-dev#1593) ci(remove future behavior warning): Set the `asyncio_default_fixture_loop_scope` to `fixture` (posit-dev#1655) bug: Verify mypy can run on CI (posit-dev#1650) Fix CI install failures on Windows (posit-dev#1651) Quartodoc 0.7.6 (posit-dev#1636) Allow `@chat.transform_assistant_response` function to return `None` (posit-dev#1641) feat: Support templates with `_template.json` metadata (posit-dev#1631) Fix KeyError when serving static files (posit-dev#1648) tests(navsets): Add navsets kitchensink tests (posit-dev#1602) Change default claude model to 3.5 sonnet
This PR refactors
shiny create
so that it now recognizes a_template.json
file. In short, every template that we create should now have an associated_template.json
.The
_template.json
for our internal templates looks like this:Edit: the
"id"
field was originally"name"
at the start of this PR, but I landed on"id"
because the value should be unique within a repo of templates. Also"name"
and"title"
are ambiguous but"id"
and"title"
are more clearly distinct.In general, only
id
is required, butid
,title
, anddescription
are expected to be the most commonly included fields. Additional fields can be added but will be ignored by the internalShinyTemplate
data type.We differentiate between
"type": "app"
templates and"type": "package"
templates, although I don't think there are external package-templates at this time. In the future,type
could be used for other templates, e.g."type": "module"
would be interesting.I've also updated the
py-shiny-templates
repo to use the_template.json
file (see posit-dev/py-shiny-templates#23), so thatshiny create
can now list templates in an external github repo:Note that the menu ordering is driven by sorted path names, so I renamed our internal templates to use
01-....
,02-....
, etc. The template"name"
in the JSON file is expected to be unique, but we don't do any validation here and instead return the first template with a matching name. We do print out confirmation of which template we're using so there is an opportunity to notice a problem in advance.Generative AI Templates
While here, I also added the generative AI examples to the default
shiny create
interface: