remove deployTFModel; Connect no longer supports hosting TensorFlow Model APIs #789
Conversation
aronatkins
force-pushed
the
aron-remove-tf
branch
from
2e211cd
to
5c04a8b
Compare
| #' @details | ||
| #' | ||
| #' @family Deployment functions | ||
| #' @export |
There was a problem hiding this comment.
Might be worth keeping this function around with an informative error? Not that important but it is nice to know that it’s definitely gone away, rather than you forgetting the name of the function.
There was a problem hiding this comment.
@kellobri - What are your thoughts about this? My feeling is that this content type saw so little use that there is almost no risk to fully removing the TF deploy function. I'm happy to retain a skeleton implementation that errs if it'll be helpful.
There was a problem hiding this comment.
Yes, we saw extremely low usage. Errors are always nice though, I like the idea of the skeleton implementation if that's possible.
| @@ -110,7 +110,7 @@ appDependencies <- function(appDir = getwd(), appFiles = NULL) { | |||
| } | |||
|
|
|||
| needsR <- function(appMetadata) { | |||
| if (appMetadata$appMode %in% c("static", "tensorflow-saved-model")) { | |||
| if (appMetadata$appMode %in% c("static")) { | |||
There was a problem hiding this comment.
Could also remove the c and switch to ==
There was a problem hiding this comment.
I considered this, but the c reminds us that there might be more than one thing that is not R. I don't feel strongly.
There was a problem hiding this comment.
I'm guessing that this function is not called for things other than static which don't need R? Perhaps it makes more sense to list the app does that do need R in that case, as it's more self-explanatory than requiring on this conditional + other conditionals elsewhere.
(I fell behind on @hadley's PRs when I had that two-week escalations stretch, so I recognize there may be context I'm missing here. I'll catch up on them when I'm done with server migration work.)
| @@ -110,7 +110,7 @@ appDependencies <- function(appDir = getwd(), appFiles = NULL) { | |||
| } | |||
|
|
|||
| needsR <- function(appMetadata) { | |||
| if (appMetadata$appMode %in% c("static", "tensorflow-saved-model")) { | |||
| if (appMetadata$appMode %in% c("static")) { | |||
There was a problem hiding this comment.
I'm guessing that this function is not called for things other than static which don't need R? Perhaps it makes more sense to list the app does that do need R in that case, as it's more self-explanatory than requiring on this conditional + other conditionals elsewhere.
(I fell behind on @hadley's PRs when I had that two-week escalations stretch, so I recognize there may be context I'm missing here. I'll catch up on them when I'm done with server migration work.)
|
Closing this PR; will push a new branch that maintains a defunct stub. |
No description provided.