R component generation - support different package structures

[tcbegley]

Hello,

I'd like to add support for dashR to dash-bootstrap-components. I hit a bit of a stumbling block though because we put the generated components in dash_bootstrap_components/_components, whereas the build pipeline in _r_components_generation.py appears to use the project shortname as the assumed location of the generated components / package.json etc.

Passing dash_bootstrap_components/_components as the project shortname doesn't resolve the issue as this is passed to importlib which then of course can't find a module by that name.

This tentative PR contains what I believe to be a minimal change that allows me to build R components for dash-bootstrap-components. It splits the package name off from the front of the path to the generated components.

I have tested that my changes don't break the build processes for dash-core-components, dash-html-components and dash-table.

One point for discussion is that I think the variable names such as project_shortname are now misleading in some places, but I left them as is for now in the interest of making minimal changes.

Anyway, very happy to discuss whether this is the right way forward or if there is a better solution.

Contributor Checklist

• [ x ] I have run the tests locally and they passed. EDIT - looks like percy is failing because of a visual change that is somewhat surprising to me. Might need a bit of assistance figuring out where that is coming from and whether my proposed changes are responsible.

@rpkyle - I think this mostly falls under your remit? All of the changes are contained to _r_components_generation.py.

[rpkyle]

@tcbegley 🎉 This is great! I think you've opened our first community PR for contributions to the R package generator. I think this should be possible without too much effort on our end.

[alexcjohnson]

Thanks very much @tcbegley! Very nice to see that these changes are all it takes to support a different structure like you have in dash_bootstrap_components.

Don't worry about that Percy diff - I actually have a fix for it in #1103, until we manage to merge that it's a known flakiness we can just approve when everything else is ready.

One point for discussion is that I think the variable names such as project_shortname are now misleading in some places, but I left them as is for now in the interest of making minimal changes.

Absolutely, I appreciate seeing the code with minimal changes but you're right that we should rename them to match their actual meanings, as well as updating their descriptions in the ArgumentParser - so that others are aware they can do what you've already done on the Python side - I didn't even realize you could do such a thing before looking at your code 🥇

And then we'll need to include tests of building a component suite using your structure. The test I have in mind doesn't quite exist using our base structure - I'd like to basically recreate a minimal repo in a subdirectory of the test dir here and call dash-generate-components on it - then verify some key things about the output before actually putting these components into an app and running it. Perhaps the right way to do this (if you're willing to bear with us for a bit) is for one of us to write that test for the base structure, then you can mimic it using your more nested structure?

[tcbegley]

I think you've opened our first community PR for contributions to the R package generator.

Oh cool! Very happy to have that distinction.

I have been looking at this a bit more, I realised there was a problem with my first attempt. In write_js_metadata the javascript bundles, CSS and source maps are copied to inst/deps, which requires the path to their location rather than the project shortname (which is what I was passing).

Perhaps the right way to do this (if you're willing to bear with us for a bit) is for one of us to write that test for the base structure, then you can mimic it using your more nested structure?

Yep that sounds like a good approach to me. I'm not in any particular rush (not least as I still have some work to do to understand how R packaging actually works in the first place...), so take all the time you need.

[rpkyle]

I think @Marc-Andre-Rivet might be able to look into this one, which would be great!

[Marc-Andre-Rivet]

As part of writing tests for this PR, I've been having a look at the behavior of the generator for both standard and nested code generation.

What I'm seeing is that the Py generator behaves fine for
dash-generate-components ./src/components my_component_package
With:

1. JS build artifacts directly in ./my_component_package
2. JS build artifacts in a nested folder like ./my_component_package/nested with the _js_dist entries using relative_package_path='nested/...'

What does not work with the Py generator is something like this:
dash-generate-components ./src/components my_component_package/nested
Arguably, I don't think it should work at all, and this is fine, as what we're really trying to do here is generate a component package named my_component_package/nested and matching both the real package name and the nested nature of the build artifacts.. the artifacts nesting should be unrelated from the wrapper's generation. eg. We shouldn't have to sanitize the package name.

For the R generator, taking into consideration that the package name shouldn't be my_component_package/nested but rather just my_component_package, the problem seems to be that inst/deps is not picking up the nested files. It seems that just updating the generator at https://github.com/plotly/dash/blob/dev/dash/development/_r_components_generation.py#L505 to do this instead:

    for f in glob.glob("{}/**/*.*".format(project_shortname), recursive=True):
        filename = os.path.basename(f)
        extension = os.path.splitext(filename)[1]

        if extension not in [".css", ".js", ".map"]:
            continue

        rel_dirname = os.path.relpath(os.path.dirname(f), project_shortname)
        target_dirname = os.path.join(os.path.join("inst/deps/", rel_dirname))

        if not os.path.exists(target_dirname):
            os.makedirs(target_dirname)

        shutil.copy(f, target_dirname)

would go a long way to ensure the R generated package has all artifacts.

@tcbegley I think what I'm suggesting here would work with your project with some minor tweaks in the __init__.py file but let me know if I'm missing something.

With the change above, I can now run:

library(dash)
library(dashHtmlComponents)
library(dashCoreComponents)
library(dashTable)
library(dashGeneratedTestComponentNested)

app <- Dash$new()
app$layout(dgtc_nestedMyComponent(id = "id", value = "nested"))

app$run_server()

This actually works but the js file seems to live in the wrong place, which is weird, assuming that something in DashR might be flattening the structure again?

Test component code can be found in https://github.com/plotly/dash/tree/pr1125-tests/%40plotly/dash-generator-test-component-nested

@alexcjohnson @rpkyle

[tcbegley]

Thanks for looking into this @Marc-Andre-Rivet, sorry for being slow to get back to you.

For reference I'm testing stuff out on a fork of dash-bootstrap-compoennts here. My current thinking is that we'll maintain a branch with the built R package that can be installed from with devtools. Anyone who wants to test can install at the moment with install_github('tcbegley/dash-bootstrap-components@r-release').

I pasted the changes you suggested into my fork of Dash and built mostly successfully. To get dashR to pick up my JavaScript bundle properly I needed to make two changes which I'm assuming are the problems you anticipated when you said I might need to modify my __init__.py.

Specifically here I changed

window["dash_bootstrap_components/_components"]

to

window["dash_bootstrap_components"]

and here I changed

script = '_components/dash_bootstrap_components.min.js'

to

script = 'dash_bootstrap_components.min.js'

Ideally I wouldn't have to change the Javascript bundle, so that the same code can be shared across Python, R, Julia versions, so I'm guessing there's probably a better way that I haven't figured out yet, but if you have a moment elaborate on what you had in mind for my __init__.py that would likely be very helpful.

EDIT - Oh, and FWIW I'm building with

dash-generate-components ./src/components dash_bootstrap_components/_components --r-prefix 'dbc'

[rpkyle]

EDIT - Oh, and FWIW I'm building with

dash-generate-components ./src/components dash_bootstrap_components/_components --r-prefix 'dbc'

@tcbegley I think the changes @Marc-Andre-Rivet has proposed will require component developers to stick with the original syntax instead, i.e.

dash-generate-components ./src/components --r-prefix 'dbc'

but these proposed modifications within _r_components_generation.py will also automatically allow arbitrary directory paths for the asset dependencies (like _components). So hopefully the component developer won't need to tell the generator about these, they'll be used as-is.

[Marc-Andre-Rivet]

@tcbegley

and here I changed
script = '_components/dash_bootstrap_components.min.js'
to
script = 'dash_bootstrap_components.min.js'

Ideally I wouldn't have to change the Javascript bundle, so that the same code can be shared across Python, R, Julia versions

Yes. Definitely don't want component authors to have to go around and tweak the generated files for the various flavors of Dash or use a different project name. Should be as transparent as possible. In the same vein, you should always be able to generate by using your project's name dash_bootstrap_components.

With the _r_components_generation.py changes suggested above, that would cause the generated file path automatically be

file = "deps"), meta = NULL,
script = 'nested/dash_generator_test_component_nested.js',

as discussed with @rpkyle it might be preferable for to generate this instead:

file = "deps/nested"), meta = NULL,
script = 'dash_generator_test_component_nested.js',

but it doesn't feel absolutely necessary.

Started some work in DashR to support nested folders correctly: https://github.com/plotly/dashR/tree/pr1125-dashR, fingerprinting and file resolution on requests is working in most cases but needs debugging for the DashR internal resources.

@rpkyle

[Marc-Andre-Rivet]

From the discussion in this PR with @rpkyle and @tcbegley, I propose supplanting this PR with #1203.

• Sanity test for component with nested (and standard non-nested) artifacts
• Update to the R component generator to include all nested files in the target folder instead of only the files directly in the current folder
• Allows component authors to use the same target name for both R and Python generation, whatever the artifacts folder structure

[Marc-Andre-Rivet]

@tcbegley Will be closing this PR as superseded by #1203 and plotly/dashR#191. Thanks for pointing out and helping understanding the issue.

In the next version of Dash, you should be able to generate your components package with the same name across all target languages with arbitrary artifact paths.

Let us know if you run into additional issues!

[tcbegley]

Thanks @Marc-Andre-Rivet! The tests you wrote were very helpful in clearing up my misunderstandings.

By copying the structure of your dash-generator-test-component-nested example I think I've managed to build everything successfully (no manual edits this time 😳), though I had a little trouble running it. Specifically dashR seems to be looking in the wrong place for the JavaScript bundle. I tried building your example too and adding that to my app to check it wasn't something wrong with my build and I got the same error:

The dependency path 'deps/dash_generator_test_component_nested.js' within the 'dashGeneratorTestComponentNested' package is invalid; cannot find 'dash_generator_test_component_nested.js'.
The dependency path 'deps/dash_bootstrap_components.min.js' within the 'dashBootstrapComponents' package is invalid; cannot find 'dash_bootstrap_components.min.js'.

Clearly the dependency paths ought to be deps/nested/... and deps/_components/... respectively, so I'm not sure what's happened to cause it to drop the nested file structure.

I ran with your changes to dashR installed by doing install_github("plotly/dashR@dev"). Is that right or should I have done something else?

[Marc-Andre-Rivet]

@tcbegley There is no code change in DashR, just added tests, so running DashR from dev shouldn't change anything. Have you tried using dash@dev to get the modified generator instead?

[tcbegley]

Yes, I installed from your branch. I'll go back and check though, maybe I just messed up my virtual environments or something. I'll let you know if it doesn't work when I try it again. Thanks!

[Marc-Andre-Rivet]

@tcbegley I've forked / played around a bit with the dbc build here:
Marc-Andre-Rivet/dash-bootstrap-components@405db54

Haven't tried to get all existing code working or anything (note the 🔪on component styles), but just to reach a point where the project builds and loads correctly in R.

Sample app:

library(dash)
library(dashHtmlComponents)
library(dashBootstrapComponents)

app <- Dash$new()
app$layout(htmlDiv(list(dbcTable(
        htmlTr(list(
            htmlTd(list("Hello World"))
        ))
))))

app$run_server()

[tcbegley]

Thanks so much @Marc-Andre-Rivet! I've had pretty limited time for this recently and you are helping me get my head around the required changes much faster than I would by myself, really appreciate it!