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 intoapache:masterfrom
jeremiedb:graphR
Mar 2, 2018
Merged

Fix R build crash in CI#9965
szha merged 3 commits intoapache:masterfrom
jeremiedb:graphR

Conversation

@jeremiedb
Copy link
Copy Markdown
Contributor

Fix for #9957
Resolves the deprecated function in DiagrammeR package used in graph.viz.
Graph nodes are now of fixed size rather than adapting to wrap the whole text, I will check for a solution to this in the next days.

@jeremiedb jeremiedb requested a review from thirdwing as a code owner March 2, 2018 19:07
@marcoabreu marcoabreu mentioned this pull request Mar 2, 2018
@marcoabreu
Copy link
Copy Markdown
Contributor

Thanks a lot!
@apache/mxnet-committers please merge this ASAP in order to unblock CI. Afterwards, please retrigger failed PRs.

@szha szha merged commit bce00aa into apache:master Mar 2, 2018
@marcoabreu
Copy link
Copy Markdown
Contributor

marcoabreu commented Mar 2, 2018 via email

Comment thread R-package/R/viz.graph.R

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

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

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

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

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

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

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.

nswamy pushed a commit to nswamy/incubator-mxnet that referenced this pull request Mar 5, 2018
* fix viz.graph R

* fix viz.graph R
nswamy pushed a commit to nswamy/incubator-mxnet that referenced this pull request Mar 5, 2018
* fix viz.graph R

* fix viz.graph R
nswamy pushed a commit to nswamy/incubator-mxnet that referenced this pull request Mar 5, 2018
* fix viz.graph R

* fix viz.graph R
marcoabreu pushed a commit that referenced this pull request Mar 6, 2018
* Fix R build crash in CI (#9965)

* fix viz.graph R

* fix viz.graph R

* revert DiagrammeR compatibility to 0.9.0 (#9975)
jinhuang415 pushed a commit to jinhuang415/incubator-mxnet that referenced this pull request Mar 30, 2018
* fix viz.graph R

* fix viz.graph R
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
* fix viz.graph R

* fix viz.graph R
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
* fix viz.graph R

* fix viz.graph R
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants