Skip to content

Remove DefaultTimeConverterIsIdentity test#1401

Closed
ddelgadovargas-cyber wants to merge 2 commits into
pytorch:mainfrom
ddelgadovargas-cyber:fix_approximate_clock_test
Closed

Remove DefaultTimeConverterIsIdentity test#1401
ddelgadovargas-cyber wants to merge 2 commits into
pytorch:mainfrom
ddelgadovargas-cyber:fix_approximate_clock_test

Conversation

@ddelgadovargas-cyber
Copy link
Copy Markdown
Contributor

@ddelgadovargas-cyber ddelgadovargas-cyber commented May 13, 2026 •
edited
Loading

The DefaultTimeConverterIsIdentity test in ApproximateClockTest.cpp was failing because it relies on the state of a global static variable returned by get_time_converter(). If other tests in the same binary run first and modify this state, the test fails because the converter is no longer the identity function.

third_party/libkineto/test/ApproximateClockTest.cpp:105: Failure
Expected equality of these values:
  converter(kTestValue)
    Which is: 0
  static_cast<libkineto::time_t>(kTestValue)
    Which is: 123456789
Stack trace:
0x7fb663e19a5a: ApproximateClockTest_DefaultTimeConverterIsIdentity_Test::TestBody() @ ??:??
0x7fb5f80e4086: testing::Test::Run() @ ??:??
0x7fb5f80e50db: testing::TestInfo::Run() @ ??:??
... Google Test internal frames ...

This test validates the identity function in its initial state, which is not very useful. This change removes the test case.

@meta-cla meta-cla Bot added the cla signed label May 13, 2026
scotts
scotts reviewed May 14, 2026
View reviewed changes
Comment thread libkineto/test/ApproximateClockTest.cpp Outdated

TEST(ApproximateClockTest, DefaultTimeConverterIsIdentity) {
auto& converter = get_time_converter();
converter = [](approx_time_t t) { return t; };
Copy link
Copy Markdown
Contributor

@scotts scotts May 14, 2026

Choose a reason for hiding this comment

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

Digging into this, I think we should just remove this test entirely. get_time_converter() being the identity function is its initial state that we expect to be modified in:

kineto/libkineto/src/GenericActivityProfiler.cpp

Line 477 in ea8bc18

get_time_converter() = clockConverter.makeConverter();
This test, then, is not actually testing anything useful.

Copy link
Copy Markdown
Contributor Author

@ddelgadovargas-cyber ddelgadovargas-cyber May 14, 2026

Choose a reason for hiding this comment

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

@scotts , I updated the PR header and removed the test case.

@ddelgadovargas-cyber ddelgadovargas-cyber changed the title Reset get_time_converter() to identity in ApproximateClockTest Remove DefaultTimeConverterIsIdentity test May 14, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented May 14, 2026

@scotts has imported this pull request. If you are a Meta employee, you can view this in D105224372.

@meta-codesync meta-codesync Bot closed this in 81d31cd May 15, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented May 15, 2026

@scotts merged this pull request in 81d31cd.

@scotts scotts mentioned this pull request May 21, 2026
Closed
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request May 22, 2026
@scotts @pytorchmergebot
Update third_party/kineto submodule to 5902263 (#184784)
cf25636 
Includes the following commits:

- ci: declare workflow-level `contents: read` on 5 workflows (pytorch/kineto#1404) 5902263
- Remove deprecated `REQUEST_TIMESTAMP` config key (pytorch/kineto#1409) 55883de
- Fix intermittent Mac CI failure from conda channel reset (pytorch/kineto#1407) ee27b5c
- Add nlohmann/json as a top-level third_party submodule (pytorch/kineto#1406) c044281
- Remove SIGUSR2 on-demand profiling path (pytorch/kineto#1408) 471ed38
- Fix ROCm HtoD memcpy stream attribution (pytorch/kineto#1398) 799b5f4
- Fix UST_LOGGER_MARK_COMPLETED build failure in manifold_trace_logger (pytorch/kineto#1389) 60967ce
- Remove `DefaultTimeConverterIsIdentity` test (pytorch/kineto#1401) 81d31cd
- Re-enable most PyTorch tests (pytorch/kineto#1403) 212f9a5
- Daily `arc lint --take CLANGFORMAT` (pytorch/kineto#1402) 6481fac
- Resolve CUPTI cbid names via cuptiGetCallbackName (pytorch/kineto#1400) e07e121
- XPUPTI: Fix ts=0 trace events on Windows (pytorch/kineto#1381) 4c8d01c
- Remove LIBKINETO_NO* compatibility shim (pytorch/kineto#1399) ea8bc18
- Upgrade Kineto to C++20 (pytorch/kineto#1397) 77e2b46
- Update the rocm api filtering (pytorch/kineto#1392) e0ac578
Pull Request resolved: #184784
Approved by: https://github.com/NicolasHug, https://github.com/malfet
pytorchmergebot pushed a commit to khushi-411/pytorch that referenced this pull request May 24, 2026
@scotts @pytorchmergebot
Update third_party/kineto submodule to 5902263 (pytorch#184784)
11669c6 
Includes the following commits:

- ci: declare workflow-level `contents: read` on 5 workflows (pytorch/kineto#1404) 5902263
- Remove deprecated `REQUEST_TIMESTAMP` config key (pytorch/kineto#1409) 55883de
- Fix intermittent Mac CI failure from conda channel reset (pytorch/kineto#1407) ee27b5c
- Add nlohmann/json as a top-level third_party submodule (pytorch/kineto#1406) c044281
- Remove SIGUSR2 on-demand profiling path (pytorch/kineto#1408) 471ed38
- Fix ROCm HtoD memcpy stream attribution (pytorch/kineto#1398) 799b5f4
- Fix UST_LOGGER_MARK_COMPLETED build failure in manifold_trace_logger (pytorch/kineto#1389) 60967ce
- Remove `DefaultTimeConverterIsIdentity` test (pytorch/kineto#1401) 81d31cd
- Re-enable most PyTorch tests (pytorch/kineto#1403) 212f9a5
- Daily `arc lint --take CLANGFORMAT` (pytorch/kineto#1402) 6481fac
- Resolve CUPTI cbid names via cuptiGetCallbackName (pytorch/kineto#1400) e07e121
- XPUPTI: Fix ts=0 trace events on Windows (pytorch/kineto#1381) 4c8d01c
- Remove LIBKINETO_NO* compatibility shim (pytorch/kineto#1399) ea8bc18
- Upgrade Kineto to C++20 (pytorch/kineto#1397) 77e2b46
- Update the rocm api filtering (pytorch/kineto#1392) e0ac578
Pull Request resolved: pytorch#184784
Approved by: https://github.com/NicolasHug, https://github.com/malfet
pytorchmergebot pushed a commit to gaurav-redhat/pytorch that referenced this pull request May 26, 2026
@scotts @pytorchmergebot
Update third_party/kineto submodule to 5902263 (pytorch#184784)
cf09a8b 
Includes the following commits:

- ci: declare workflow-level `contents: read` on 5 workflows (pytorch/kineto#1404) 5902263
- Remove deprecated `REQUEST_TIMESTAMP` config key (pytorch/kineto#1409) 55883de
- Fix intermittent Mac CI failure from conda channel reset (pytorch/kineto#1407) ee27b5c
- Add nlohmann/json as a top-level third_party submodule (pytorch/kineto#1406) c044281
- Remove SIGUSR2 on-demand profiling path (pytorch/kineto#1408) 471ed38
- Fix ROCm HtoD memcpy stream attribution (pytorch/kineto#1398) 799b5f4
- Fix UST_LOGGER_MARK_COMPLETED build failure in manifold_trace_logger (pytorch/kineto#1389) 60967ce
- Remove `DefaultTimeConverterIsIdentity` test (pytorch/kineto#1401) 81d31cd
- Re-enable most PyTorch tests (pytorch/kineto#1403) 212f9a5
- Daily `arc lint --take CLANGFORMAT` (pytorch/kineto#1402) 6481fac
- Resolve CUPTI cbid names via cuptiGetCallbackName (pytorch/kineto#1400) e07e121
- XPUPTI: Fix ts=0 trace events on Windows (pytorch/kineto#1381) 4c8d01c
- Remove LIBKINETO_NO* compatibility shim (pytorch/kineto#1399) ea8bc18
- Upgrade Kineto to C++20 (pytorch/kineto#1397) 77e2b46
- Update the rocm api filtering (pytorch/kineto#1392) e0ac578
Pull Request resolved: pytorch#184784
Approved by: https://github.com/NicolasHug, https://github.com/malfet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scotts scotts Awaiting requested review from scotts

Assignees

No one assigned

Labels

cla signed Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ddelgadovargas-cyber @scotts