Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

Fix R build crash in CI #9965

Merged
szha merged 3 commits into from
Mar 2, 2018
Merged

Fix R build crash in CI #9965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e065631
fix viz.graph R
jeremiedb Mar 2, 2018
e8c6e1f
fix viz.graph R
jeremiedb Mar 2, 2018
78c76e5
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
jeremiedb Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R-package/DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ BugReports: https://github.com/dmlc/mxnet/issues
Imports:
methods,
Rcpp (>= 0.12.1),
DiagrammeR (>= 0.9.0),
DiagrammeR (>= 1.0.0),
visNetwork (>= 1.0.3),
data.table,
jsonlite,
Expand Down
3 changes: 1 addition & 2 deletions R-package/R/viz.graph.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#' @importFrom stringr str_trim
#' @importFrom jsonlite fromJSON
#' @importFrom DiagrammeR create_graph
#' @importFrom DiagrammeR set_global_graph_attrs
#' @importFrom DiagrammeR add_global_graph_attrs
#' @importFrom DiagrammeR create_node_df
#' @importFrom DiagrammeR create_edge_df
Expand Down Expand Up @@ -143,7 +142,7 @@ graph.viz <- function(symbol, shape=NULL, direction="TD", type="graph", graph.wi
}

graph<- create_graph(nodes_df = nodes_df_new, edges_df = edge_df_new, directed = TRUE) %>%
set_global_graph_attrs("layout", value = "dot", attr_type = "graph") %>%
add_global_graph_attrs("layout", value = "dot", attr_type = "graph") %>%
Copy link
Copy Markdown
Member

@terrytangyuan terrytangyuan Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the ideal fix though IMO. MXNet should still be able to run under DiagrammeR = 0.9.0. You should be able to check the version and trigger the methods properly based on different versions of DiagrammeR.

Copy link
Copy Markdown
Member

@terrytangyuan terrytangyuan Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will potentially lose users who cannot afford to upgrade their DiagrammeR at the moment due to large codebases.

Copy link
Copy Markdown
Contributor

@marcoabreu marcoabreu Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it rather make sense to pin the version to 0.9.0 - 0.9.2?

Copy link
Copy Markdown
Contributor

@marcoabreu marcoabreu Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or what do you think, @jeremiedb ? 0.9 sounds like an unstable beta version, so maybe it would make sense to tell users to upgrade? But we would also have to consider SemVer, so this might be difficult.

Copy link
Copy Markdown
Member

@terrytangyuan terrytangyuan Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree that works in this case but I guess I was just commenting in general that this should be something to consider.

Copy link
Copy Markdown
Contributor Author

@jeremiedb jeremiedb Mar 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that I may have been too conservative with the 1.0.0 requirement. I just made a slight modification and graphs can now be rendered as before even on 0.9.0.

add_global_graph_attrs("rankdir", value = direction, attr_type = "graph")

if (type=="vis"){
Expand Down