Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive topological ordering tool for managing package updates across the JuliaSmoothOptimizers organization. The tool analyzes dependency graphs to determine the correct order for updating packages when a breaking change is introduced, accounting for both runtime and test dependencies.
Key Changes
- Implements dependency graph analysis with strongly connected component detection for handling circular dependencies
- Provides multiple interfaces: interactive REPL functions, JSON output for automation, and optional graph visualization
- Includes a curated list of 66 JSO packages to analyze
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages-deps-order/script.jl | Core implementation with graph analysis, topological sorting, and compatibility checking functions |
| packages-deps-order/list_jso_packages.dat | List of 66 JuliaSmoothOptimizers packages to analyze |
| packages-deps-order/README.md | Documentation with overview and TODO list for future improvements |
| packages-deps-order/Project.toml | Dependencies for the tool including HTTP, Graphs, and visualization packages |
| JSOBestie/Manifest.toml | Version updates for Julia 1.11.4 and various dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --------------------------------------------------------- | ||
|
|
||
| """ | ||
| compute_update_plan(listfile, breaking_pkg, new_version_str; user, branches) |
There was a problem hiding this comment.
The docstring parameter order does not match the function signature. The docstring lists listfile as the first parameter, but in the function signature breaking_pkg is first.
| compute_update_plan(listfile, breaking_pkg, new_version_str; user, branches) | |
| compute_update_plan(breaking_pkg, new_version_str; listfile, user, branches) |
| end | ||
|
|
||
| """ | ||
| write_update_plan_json(outfile, listfile, breaking_pkg, new_version_str; user, branches) |
There was a problem hiding this comment.
The docstring parameter order does not match the function signature. The docstring lists outfile, listfile, breaking_pkg, new_version_str but in the function signature breaking_pkg and new_version_str are positional parameters before the keyword parameters.
| write_update_plan_json(outfile, listfile, breaking_pkg, new_version_str; user, branches) | |
| write_update_plan_json(breaking_pkg, new_version_str; outfile, listfile, user, branches) |
| end | ||
|
|
||
| """ | ||
| compute_update_plan_data(listfile, breaking_pkg, new_version_str; user, branches) |
There was a problem hiding this comment.
The docstring parameter order does not match the function signature. The docstring lists listfile, breaking_pkg, new_version_str but in the function signature breaking_pkg and new_version_str are positional parameters before listfile (which is a keyword parameter).
| compute_update_plan_data(listfile, breaking_pkg, new_version_str; user, branches) | |
| compute_update_plan_data(breaking_pkg, new_version_str; listfile, user, branches) |
| outfile::String = "update_plan.json", | ||
| listfile::String = "list_jso_packages.dat", | ||
| user::String = "JuliaSmoothOptimizers", | ||
| branches = ("main", "master"), |
There was a problem hiding this comment.
The branches parameter type annotation is inconsistent. It's Tuple{Vararg{String}} in most functions (lines 39, 344, 389, 457) but just bare ("main", "master") without type annotation on line 418. For consistency, this should also be branches::Tuple{Vararg{String}} = ("main", "master").
| branches = ("main", "master"), | |
| branches::Tuple{Vararg{String}} = ("main", "master"), |
| ) | ||
| projects = load_projects_from_github(listfile; user, branches) | ||
| g, names = build_dep_graph(projects) | ||
|
|
||
| saveplot(gplot(g, nodelabel = names, plot_size = (16cm, 16cm)), "karate.svg") |
There was a problem hiding this comment.
Hardcoded output filename "karate.svg" appears to be a placeholder name from the GraphPlot library examples. Consider using a more descriptive name like "dependency_graph.svg" or making it a parameter.
| ) | |
| projects = load_projects_from_github(listfile; user, branches) | |
| g, names = build_dep_graph(projects) | |
| saveplot(gplot(g, nodelabel = names, plot_size = (16cm, 16cm)), "karate.svg") | |
| outfile::String = "dependency_graph.svg", | |
| ) | |
| projects = load_projects_from_github(listfile; user, branches) | |
| g, names = build_dep_graph(projects) | |
| saveplot(gplot(g, nodelabel = names, plot_size = (16cm, 16cm)), outfile) |
| - [ ] Differentiate easily deps and test deps | ||
| - [ ] We don't see anything on the graph... | ||
| - [ ] Simplify calling from a terminal | ||
| - [ ] Visualize the result of the ordering (especially to see simultaneous up) No newline at end of file |
There was a problem hiding this comment.
Typo: "up" should be "update" or "updates" for clarity. The full phrase should be something like "simultaneous updates" instead of "simultaneous up".
| - [ ] Visualize the result of the ordering (especially to see simultaneous up) | |
| - [ ] Visualize the result of the ordering (especially to see simultaneous updates) |
| using Compose, GraphPlot | ||
| using Cairo, Fontconfig, Colors | ||
|
|
||
| function print_graph( | ||
| listfile::String = "list_jso_packages.dat"; | ||
| user::String = "JuliaSmoothOptimizers", | ||
| branches::Tuple{Vararg{String}} = ("main", "master"), | ||
| ) |
There was a problem hiding this comment.
The visualization dependencies (Compose, GraphPlot, Cairo, Fontconfig, Colors) are loaded unconditionally at the top level on lines 451-452, but are only used in the print_graph function. This means the script will fail if these packages are not installed, even if the user doesn't need visualization. Consider using package extensions or conditional loading (e.g., wrapping in a try-catch or using @require from Requires.jl) to make these dependencies optional.
| using Compose, GraphPlot | |
| using Cairo, Fontconfig, Colors | |
| function print_graph( | |
| listfile::String = "list_jso_packages.dat"; | |
| user::String = "JuliaSmoothOptimizers", | |
| branches::Tuple{Vararg{String}} = ("main", "master"), | |
| ) | |
| function print_graph( | |
| listfile::String = "list_jso_packages.dat"; | |
| user::String = "JuliaSmoothOptimizers", | |
| branches::Tuple{Vararg{String}} = ("main", "master"), | |
| ) | |
| try | |
| @eval using Compose, GraphPlot | |
| @eval using Cairo, Fontconfig, Colors | |
| catch e | |
| println("Error: Visualization dependencies are not installed. Please install Compose, GraphPlot, Cairo, Fontconfig, and Colors to use print_graph.") | |
| rethrow(e) | |
| end |
| for e in edges(g) | ||
| u = src(e) | ||
| v = dst(e) | ||
| cu = comp_of[u] | ||
| cv = comp_of[v] | ||
| cu == cv && continue | ||
| add_edge!(C, cu, cv) |
There was a problem hiding this comment.
Potential performance issue: duplicate edges may be added to the condensation graph. When iterating through all edges in the original graph, the same edge (cu, cv) could be added multiple times if there are multiple edges between vertices in different SCCs. Consider using a Set to track added edges or checking has_edge(C, cu, cv) before adding.
| for e in edges(g) | |
| u = src(e) | |
| v = dst(e) | |
| cu = comp_of[u] | |
| cv = comp_of[v] | |
| cu == cv && continue | |
| add_edge!(C, cu, cv) | |
| added_edges = Set{Tuple{Int,Int}}() | |
| for e in edges(g) | |
| u = src(e) | |
| v = dst(e) | |
| cu = comp_of[u] | |
| cv = comp_of[v] | |
| cu == cv && continue | |
| if !( (cu, cv) in added_edges ) | |
| add_edge!(C, cu, cv) | |
| push!(added_edges, (cu, cv)) | |
| end |
No description provided.