-
Notifications
You must be signed in to change notification settings - Fork 26
Improve response parsing: no ptypes, faster datetimes #513
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
base: main
Are you sure you want to change the base?
Changes from all commits
385d5cf
61f93be
b2537b4
04b0840
14518ee
ced1d4f
f83255d
6cffbef
770d862
9806f1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,7 +87,6 @@ Collate: | |
| 'page.R' | ||
| 'parse.R' | ||
| 'promote.R' | ||
| 'ptype.R' | ||
| 'remote.R' | ||
| 'runtime-caches.R' | ||
| 'schedule.R' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -788,7 +788,23 @@ get_jobs <- function(content) { | |
| validate_R6_class(content, "Content") | ||
|
|
||
| jobs <- content$jobs() | ||
| parse_connectapi_typed(jobs, connectapi_ptypes$jobs, strict = TRUE) | ||
| out <- parse_connectapi_typed( | ||
| jobs, | ||
| datetime_cols = c("start_time", "end_time", "last_heartbeat_time", "queued_time") | ||
| ) | ||
|
|
||
| # The older /applications/ endpoint returns timestamps as Unix epoch integers | ||
| # and ID fields as integers. Normalize to match the v1 endpoint's types. | ||
| # For the v1 endpoint these are already character/POSIXct, so the coercions | ||
| # are no-ops. | ||
| out <- coerce_epoch_to_posixct( | ||
| out, | ||
| c("start_time", "end_time", "last_heartbeat_time", "queued_time") | ||
| ) | ||
| coerce_to_character( | ||
| out, | ||
| c("id", "ppid", "pid", "app_id", "content_id", "variant_id", "bundle_id") | ||
| ) | ||
| } | ||
|
|
||
| #' Terminate Jobs | ||
|
|
@@ -832,22 +848,19 @@ terminate_jobs <- function(content, keys = NULL) { | |
| keys <- all_jobs[all_jobs$status == 0, ]$key | ||
| if (length(keys) == 0) { | ||
| message("No active jobs found.") | ||
| return(vctrs::vec_ptype(connectapi_ptypes$job_termination)) | ||
| return(tibble::tibble()) | ||
| } | ||
| } | ||
|
|
||
| res <- purrr::map(keys, content$register_job_kill_order) | ||
| res_content <- purrr::map(res, httr::content) | ||
| res_df <- tibble::tibble( | ||
| parse_connectapi_typed( | ||
| res_content, | ||
| connectapi_ptypes$job_termination, | ||
| strict = TRUE | ||
| ) | ||
| ) | ||
| res_df <- parse_connectapi_typed(res_content) | ||
| # Errors will not have the job_key. | ||
| res_df$job_key <- keys | ||
| res_df | ||
| # Keep only the columns relevant to job termination; the API response | ||
| # includes extra fields (e.g. payload, guid) on error that vary by outcome. | ||
| keep <- c("app_id", "app_guid", "job_key", "job_id", "result", "code", "error") | ||
|
Comment on lines
+860
to
+862
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it a problem that we get variable data at this point?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that the variable data is for error cases (i.e. trying to terminate a job that is not currently active, which returns a 409 but doesn't raise an R error) vs. successful requests. I guess it's debatable whether we should be raising an R error more eagerly, but I think the behavior of accommodating different field names is consistent with main. The one difference here is that main will always return the columns of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was actually thinking a bit in the opposite direction: if we sometimes get different fieldnames that's probably totally ok. This is probably more important (or really, more possible) when we get to having these objects not be DFs that get passed around. Then the normalization of "here are the columns you're getting" can be done at the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha yeah, if we were returning a list then I agree it'd make more sense to not worry about the columns -- the responses are what they are, and that may include different fields. |
||
| res_df[, keep, drop = FALSE] | ||
| } | ||
|
|
||
| #' @rdname get_jobs | ||
|
|
@@ -896,7 +909,7 @@ get_log <- function(job, max_log_lines = NULL) { | |
| v1_url("content", job$app_guid, "jobs", job$key, "log"), | ||
| query = query | ||
| ) | ||
| parse_connectapi_typed(res$entries, connectapi_ptypes$job_log) | ||
| parse_connectapi_typed(res$entries, datetime_cols = "timestamp") | ||
| } | ||
|
|
||
| #' Set RunAs User | ||
|
|
@@ -1141,7 +1154,8 @@ get_bundles <- function(content) { | |
| validate_R6_class(content, "Content") | ||
| bundles <- content$get_bundles() | ||
|
|
||
| parse_connectapi_typed(bundles, connectapi_ptypes$bundles) | ||
| out <- parse_connectapi_typed(bundles, datetime_cols = "created_time") | ||
| coerce_fs_bytes(out, "size") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can keep this here, but I do wonder if providing these as |
||
| } | ||
|
|
||
| #' @rdname get_bundles | ||
|
|
@@ -1347,7 +1361,7 @@ get_group_permission <- function(content, guid) { | |
| get_content_permissions <- function(content, add_owner = TRUE) { | ||
| validate_R6_class(content, "Content") | ||
| res <- content$permissions(add_owner = add_owner) | ||
| parse_connectapi_typed(res, connectapi_ptypes$permissions) | ||
| parse_connectapi_typed(res) | ||
| } | ||
|
|
||
| #' Render a content item. | ||
|
|
@@ -1495,7 +1509,7 @@ content_restart <- function(content) { | |
| get_content_packages <- function(content) { | ||
| error_if_less_than(content$connect$version, "2025.01.0") | ||
| res <- content$packages() | ||
| parse_connectapi_typed(res, connectapi_ptypes$content_packages) | ||
| parse_connectapi_typed(res) | ||
| } | ||
|
|
||
| #' Search for content on the Connect server | ||
|
|
@@ -1627,5 +1641,8 @@ as.data.frame.connect_content_list <- function( | |
| #' @export | ||
| as_tibble.connect_content_list <- function(x, ...) { | ||
| content_data <- purrr::map(x, "content") | ||
| parse_connectapi_typed(content_data, connectapi_ptypes$content) | ||
| parse_connectapi_typed( | ||
| content_data, | ||
| datetime_cols = datetime_columns$content | ||
| ) | ||
| } | ||
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 probably not worth too much digging, but if you know off the top of your head: how old are older versions here?
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.
GET /v1/content/{guid}/jobswas added in October 2022 https://docs.posit.co/connect/news/#rstudio-connect-2022.10.0; the old applications endpoint was removed July 2025.