Update check_modular_conversion #37456
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. |
| @@ -23,7 +23,7 @@ | |||
| os.path.join(MODEL_ROOT, "rt_detr", "modular_rt_detr.py"), | |||
| os.path.join(MODEL_ROOT, "qwen2", "modular_qwen2.py"), | |||
| os.path.join(MODEL_ROOT, "qwen3", "modular_qwen3.py"), | |||
| os.path.join(MODEL_ROOT, "qwen3", "modular_qwen3_moe.py"), | |||
| os.path.join(MODEL_ROOT, "qwen3_moe", "modular_qwen3_moe.py"), | |||
There was a problem hiding this comment.
It looks like this test is not running on CI or failed.
|
@qubvel Thank you for working on this. I have admitted that I have to check the sort and dependency stuff here to get better understanding before I can judge correctly. However I have a question: since we are running with multiple processes and each would modify some modeling files (i.e. generate them from modular files), it's not very clear to me that we are free of the race condition. Maybe this won't happen from the way we (and you) handle the dependency and the control flow, but I find it's kind difficult to see the logic clearly. I will leave some comments and questions in the PR changes so we can discuss in more specific positions. |
| # 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] | ||
| return sorting_list |
There was a problem hiding this comment.
it seems to me that the return type is changed from a list of string (of modular file paths) to list[list[str]].
Would be nice to explain this (i.e. the algorithm), and having a docstring about it (which is also missing on main).
There was a problem hiding this comment.
The caller (find_priority_list) to topological_sort still have ordered_files as variable, and its docstring is still A tuple with the ordered files (list). If I don't make mistake, this is no longer the case. So should update there too?
| console.print(f"[bold yellow]Files per level: {tuple([len(x) for x in ordered_files])}[/bold yellow]") | ||
|
|
||
| try: | ||
| for dependency_level_files in ordered_files: |
There was a problem hiding this comment.
at this moment, it's not easy to understand dependency_level_files and the what ordered_files is
| if not args.check_all and guaranteed_no_diff(file_path, dependencies, models_in_diff): | ||
| skipped_models.add(file_path.split("/")[-2]) # save model folder name | ||
| else: | ||
| files_to_check.append(file_path) |
There was a problem hiding this comment.
so files_to_check a list of of modular file paths and there won't be any duplicated elements?
|
Thanks for the review, @ydshieh. That's definitely a fair point. I will add more comments to clarify the algorithm! |
What does this PR do?
cc @ydshieh