Skip to content

Conversation

@ProExpertProg
Copy link

@ProExpertProg ProExpertProg commented Nov 6, 2025

Purpose

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
…itional_compilation_ranges

Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
…itional_compilation_ranges

Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Signed-off-by: ilmarkov <markovilya197@gmail.com>
Copy link
Author

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

A few initial thoughts. Could we also use a dataclass instead of a tuple for a compiled range? We can add utility methods (like is_single_size), names to the elements, and docs to make the code clearer.

Also, currently if there is more than one range above the cudagraphs capture size, I think we don't ever trigger it in the GPU model runner as the compilation only happens as the compiled model is invoked with the shape (with _dummy_run) - we should make sure to dummy run for each compile range.

I also think we should try to give hints to Inductor about the range, can be done as a follow-up.

elapsed = now - compilation_start_time
compilation_config.compilation_time += elapsed
if runtime_shape is None:
if compile_range is None:
Copy link
Author

Choose a reason for hiding this comment

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

Lost the compilation time update

Suggested change
if compile_range is None:
compilation_config.compilation_time += elapsed
if compile_range is None:

"""Sizes to compile for inductor. In addition
to integers, it also supports "cudagraph_capture_sizes" to
specify the sizes for cudagraph capture."""
compile_ranges_split_points: list[int] | None = None
Copy link
Author

Choose a reason for hiding this comment

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

This comment implies ranges are done inclusive-exclusive but in the code you use inclusive-inclusive. Can we standardize on inclusive-exclusive?

state = {
"ranges": self.ranges,
}
return InductorPass.hash_dict(state)
Copy link
Author

Choose a reason for hiding this comment

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

Add the current range to cache key and check the number of times the manager gets called (to make sure the bug you found doesn't manifest)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants