Skip to content

ProfilingSettings takes a nonnegative_int and a positive_int#1657

Open
elliottslaughter wants to merge 2 commits into
flexflow:masterfrom
elliottslaughter:profiling-iter-positive
Open

ProfilingSettings takes a nonnegative_int and a positive_int#1657
elliottslaughter wants to merge 2 commits into
flexflow:masterfrom
elliottslaughter:profiling-iter-positive

Conversation

@elliottslaughter

@elliottslaughter elliottslaughter commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

This splits out the first part of #1641 so that we can start to merge pieces without taking the whole monolithic ~4,000 line diff all at once.

This adds constraints on ProfilingSettings by typing the fields more accurately to nonnegative_int and positive_int respectively. Previously we had a lot of tests that simply weren't running due to passing zero for both parameters.

The impact of this change is that tests may start running that were previously effectively being no-oped out. Thus, if you merge this before #1640 we may see failures in that PR, and vice versa if you merge #1640 first then you may discover that it actually broke something silently (because some tests may not actually be exercised right now).

This passes CPU and GPU tests on master as long as you merge #1656 first (an otherwise unrelated change).


This change is Reviewable

@elliottslaughter

Copy link
Copy Markdown
Collaborator Author

So it occurs to me, re-reading this code, that I'm not sure this is the right fix. The original code clearly is intended to permit a no-op when measure_iters <= 0. That seems like a footgun to me, but if there are situations where we legitimately want this feature (e.g., maybe we profile some parts of the graph while no-oping others?) then this change essentially makes those use cases impossible by design.

@lockshaw I guess up to you whether you think this is a useful change or not. The other option is to just fix the call sites (in which case new users might continue to make the same mistake, setting measure_iters to zero unintentionally).

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.

1 participant