Fix modular edge case + modular sorting order #35562
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Cyrilvallez
force-pushed
the
modular-regex
branch
from
54e9d2d to
bdbb053
Compare
Cyrilvallez
changed the title
| def topological_sort(dependencies: dict): | ||
| # Nodes are the name of the models to convert (we only add those to the graph) | ||
| nodes = {node.rsplit("modular_", 1)[1].replace(".py", "") for node in dependencies.keys()} | ||
| # This will be a graph from models to convert, to models to convert that should be converted before (as they are a dependency) | ||
| graph = {} | ||
| name_mapping = {} | ||
| for node, deps in dependencies.items(): | ||
| node_name = node.split("/")[-2] | ||
| for dep in deps: | ||
| dep_name = dep.split(".")[-2] | ||
| if dep_name == node_name: | ||
| # Skip self dependencies for topological sort as they create cycles | ||
| continue | ||
| if "example" not in node and "auto" not in dep and node_name not in graph[dep_name]: | ||
| graph[dep_name].append(node_name) | ||
| new_dependencies[node_name] = node | ||
|
|
||
| # Create a graph and in-degree count for each node | ||
| def filter_one_by_one(filtered_list, reverse): | ||
| if len(reverse) == 0: | ||
| return filtered_list | ||
|
|
||
| graph = defaultdict(list) | ||
| # Build the graph | ||
| for node, deps in reverse.items(): | ||
| for dep in deps: | ||
| graph[dep].append(node) | ||
|
|
||
| base_modules = set(reverse.keys()) - set(graph.keys()) | ||
| if base_modules == reverse.keys(): | ||
| # we are at the end | ||
| return filtered_list + list(graph.keys()) | ||
| to_add = [] | ||
| for k in graph.keys(): | ||
| if len(graph[k]) == 1 and graph[k][0] in base_modules: | ||
| if graph[k][0] in reverse: | ||
| del reverse[graph[k][0]] | ||
| if k not in filtered_list: | ||
| to_add += [k] | ||
| for k in base_modules: | ||
| if k not in filtered_list: | ||
| to_add += [k] | ||
| filtered_list += list(to_add) | ||
| return filter_one_by_one(filtered_list, reverse) | ||
|
|
||
| final_order = filter_one_by_one([], graph) | ||
|
|
||
| return [new_dependencies.get(k) for k in final_order if k in new_dependencies] | ||
| node_name = node.rsplit("modular_", 1)[1].replace(".py", "") | ||
| dep_names = {dep.split(".")[-2] for dep in deps} | ||
| dependencies = {dep for dep in dep_names if dep in nodes and dep != node_name} | ||
| graph[node_name] = dependencies | ||
| name_mapping[node_name] = node | ||
|
|
||
| sorting_list = [] | ||
| while len(graph) > 0: | ||
| # Find the nodes with 0 out-degree | ||
| leaf_nodes = {node for node in graph if len(graph[node]) == 0} | ||
| # Add them to the list | ||
| sorting_list += list(leaf_nodes) | ||
| # Remove the leafs from the graph (and from the deps of other nodes) | ||
| graph = {node: deps - leaf_nodes for node, deps in graph.items() if node not in leaf_nodes} | ||
|
|
||
| return [name_mapping[x] for x in sorting_list] |
There was a problem hiding this comment.
Congrats, much easier to read and understand from an (almost) outsider perspective. maybe naive notes/questions:
- there can't be dependency cycles, right? because they'd break this, the loop would not end
- I'd be wary of sets since they don't preserve ordering, we might want it later
There was a problem hiding this comment.
Hey @molbap, thanks for looking into it! You raise some good points, I should have provided a bit more details:
-
Indeed in our case we can never have cycles as models are added chronologically, ensuring it (if at time 't', i.e. now, we dont' have cycles, we will never have any by adding more models: adding a model is equivalent to adding a new non existing node with 0 in-degree and as much out-degree as modeling imported). For our past model refactors, we might need to be slightly wary of this; it is our responsibility to ensure consistency here and not add cycles. But I'm extremely confident this case will never appear in practice, as we usually rely on the old "copied from" to know from which model we can refactor, which has the same notion of chronology. Worst case scenario, the person refactoring an old model and adding a cycle by mistake will immediately notice that there is a bug, and will be able to switch the "origin model" for the refactor.
Thus, the topology of our current (and future) graph (directed acyclic graph) ensures that the algorithm will not get stuck 👌🏻 -
At each step, the order of the nodes with 0 out-degree does not matter, so it is easier to use sets here as they have nice property and we can easily take the difference with the "-" operator!
There was a problem hiding this comment.
- well put, thanks! understood, yes I was thinking people might get caught up in an error during development but it will never reach main. Dag rules
- if we're sure order is useless then yes, sets are the best choice indeed :)
There was a problem hiding this comment.
LGTM nice upgrade haha I remember trying super hard to make it recursive for no reasons.
Before merging, we need a test to make sure we don't break anything. I was too lazy to add it, sorry that it falls on you! 😓 (let's explicitly write the order we expect for given deps by using current state of the repo!)
What does this PR do?
Fix a very niche edge-case discovered in #35147: if the model name appear in a normal word (e.g.
samappears in the worddownsample, then it is replaced by the new name). This is of course unwanted, and a simple check for previous char in the regex fixes it.Also fix the
topological_sortfunction increate_dependency_mapping.py: it is currently fully skipping some files that are silently not converted. I had a hard time finding the exact root, so I mostly rewrote it (I believe it is much simpler now), and it works nicely. I also added a check inmodular_model_converter.pyto ensure that we never silently skip files.Finally, I added back the possibility to convert the examples. Note that this does not slow down
make fix-copies, which I think was the reason to remove them, as they are not included incheck_modular_conversion.py(and it's important that we are able to check on them as well). They will not be converted when doingpython utils/modular_model_converter.py --files_to_parse allthough, I added the keywordpython utils/modular_model_converter.py --files_to_parse examples.cc @ArthurZucker and @yonigozlan as you touched it recently as well