Sage-Bionetworks / dccvalidator-app

0 stars 0 forks source link

Possible approach to separating deployment of the app from the dccvalidator package #45

Closed karawoo closed 3 years ago

karawoo commented 3 years ago

As discussed in our team meeting, here is my thinking for how we could split our deployment of the dccvalidator app from the package that provides the functionality. The most important pieces here are app.R and config.yml. The config file is almost identical to the one we currently have in the dccvalidator repo, with the one exception that the documentation page is now referenced by finding its location within the installed package, not its location in the file hierarchy of the repo. The distinction between these has been a source of confusion in the past (see e.g. the discussion on this PR).

Everything else here exists to control the R and Python dependencies the app uses. For Python dependencies, we have a requirements.txt and a shell script that sets up a virtualenv; for R dependencies we use renv. Ideally we'd have a dockerfile to handle this, but the shiny server doesn't support that.

My goal here would be to standardize around using the package as a package: we would have one official, supported way to spin up an instance of the app, and that would be to install the package and call the run_app() function. We would not have to support the two different ways of deploying the app that we do now, and we'd avoid confusions like the one in the PR above when things work in one way but not the other. We would also cease having dccvalidator be its own dependency, which conflicts with how tools like renv expect R projects to be structured (update 2020-10-16 renv has added back the ability to snapshot your own project, so this is no longer a blocker to us upgrading our renv version, though it is still a bit circular).

This proposal would make it take slightly more steps to incorporate dccvalidator's changes into the live version of the app, in that you would need to update the dccvalidator version in this repo after a change was merged in the dccvalidator repo. We already sometimes have to do this within the dccvalidator repo after a PR is merged, however (e.g. https://github.com/Sage-Bionetworks/dccvalidator/pull/248).

It's also possible that a change in dccvalidator would add a new configuration option, and storing a config file separately here might make it easier to miss the fact that we need to update the config file.

Looking forward to hearing more thoughts!

Aryllen commented 3 years ago

This is really interesting. I'm going to let this stew and will most likely have more comments/questions next week.

karawoo commented 3 years ago

I'm going to close this since we decided not to move forward with it, and instead to incorporate new golem updates which might address some of our issues.