Dynamic timeline #2215

Vikas-kum · 2020-08-28T02:01:12Z

This PR
1)introduces a new Python API for starting and stopping the timeline:

hvd.start_timeline('/path/to/timeline.json', mark_cycles=True)
...
hvd.stop_timeline()

Makes sure that timeline json file is valid json.
Including starttime as metadata in timeline.

TODO
1/ thread detach() re-introduce.

Fixes #2008.
This is followup from this PR: #2129 and addresses concurrent access.

Checklist before submitting

Did you read the contributor guide?
Did you update the docs?
Did you write any tests to validate this change?
Did you update the CHANGELOG, if this change affects users?

Description

Fixes # (issue).

Review process to land

All tests and other checks must succeed.
At least one member of the technical steering committee must review and approve.
If any member of the technical steering committee requests changes, they must be addressed.

tgaddair · 2020-08-29T17:42:25Z

Nice work, @Vikas-kum! Thanks for picking this up. I'll take a look today or tomorrow.

tgaddair

Nice work! A few questions.

horovod/common/timeline.cc

tgaddair · 2020-09-01T21:00:19Z

horovod/common/timeline.cc

+  } else {
+    // last event closed the json ']' , need to seek to one position back and write ',' to continue
+    long pos = file_.tellp();
+    file_.seekp (pos-1);


Is there any performance penalty to doing this? Timeline already introduces a lot of overhead, especially when using markers. Can you test it with synthetic benchmark with and without this op?

I dont think it should cause any perf penalty, given that we are already going to write bytes from next position.
Would you be able to help me with quickly running a small job and verifying on this PR as I will ahve to go through setups, which i dont currently have. :) I know I am asking for a favor but will appreciate if you can help.

test/test_torch.py

horovod/common/operations.cc

Vikas-kum · 2020-09-03T21:43:35Z

Thanks @tgaddair for review. Will update PR soon addressing comments.

Vikas-kum · 2020-09-09T18:42:03Z

@tgaddair Gentle reminder. Would you be take another round at this.

tgaddair

The more I think about, I believe your approach here makes sense then what I was trying to achieve in my last couple of commits to #2129, as we keep all the state modification in the RunLoopOnce, which avoids race conditions.

The one thing I would still like to see is to make these calls blocking so the user doesn't need to sleep after each call (otherwise, they could miss allreduce information on the timeline when starting, or the end of the timeline if they stop and then exit the program). Does that sound reasonable to you?

Vikas-kum · 2020-09-10T21:17:39Z

The one thing I would still like to see is to make these calls blocking so the user doesn't need to sleep after each call (otherwise, they could miss allreduce information on the timeline when starting, or the end of the timeline if they stop and then exit the program). Does that sound reasonable to you?

I took this approach because it simplifies the implementation quite a bit and also makes sure that locks are held for much smaller and confined scope.
We can make stop timeline blocking, but even then we would need sleep in test code to ensure that events are written.
Say you start_timeline

Start takes into effect only when BGThread goes to next iteration. To make sure of correctness test checks for cycle markers and events getting captured we would need to wait for at least 2 full cycle, so that event and cycle markers are indeed written.

hvd.start_Timeline()--> (sleep here in test code makes sure that timeline is started before next line test code)starts takes in to effect --> allreduce (event will be captured) --> events are written and cycles are written to queue --> stop_timeline(No more events)--> WriterThread dump any existing events to a file.

Somehow, I don't feel strongly about the idea of blocking call because it blocks my training script completely. I do see a side effect of having non-blocking calls and that is actual events may be written in next cycle of thread. But at this point, I expect user would be calling start_timeline to stop_timeline for sufficient gap which gives them a good picture of what is happening. If user wants to block, they can put this kind of sleep 1s(and know that they are knowingly blocking their script) in their code to make sure that events get dumped.

For this reason, I would like to have this non-blocking. Please let me know if you have strong feeling about blocking call and I can make stop timeline blocking.

Signed-off-by: Travis Addair <taddair@uber.com>

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Adding mroe test Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

remove debug statement addressed comments Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

tgaddair · 2020-09-15T01:43:49Z

Thanks for making these changes, @Vikas-kum!

Last thing I would ask: can run clang-format from the root of the project to make sure code style aligns with the rest of the code base?

If tests pass after that, we should be good to land :)

Vikas-kum · 2020-09-15T19:35:12Z

Thanks for making these changes, @Vikas-kum!

Last thing I would ask: can run clang-format from the root of the project to make sure code style aligns with the rest of the code base?

If tests pass after that, we should be good to land :)

thanks @tgaddair , I ran clang-format for only timeline.h timeline.cc
I see running clang-format for other files introduces a lot of formatting changes, unrelated to this PR(See here if I run clang-format on horovod-common dir https://github.com/horovod/horovod/pull/2278/files). So, I would suggest that we should have different PR just with clang-format run. WDYT?

tgaddair

Sounds good, will take a look at the follow-up PR!

Vikas-kum · 2020-09-15T23:04:05Z

Sounds good, will take a look at the follow-up PR!

Thanks. Created this PR: https://github.com/horovod/horovod/pull/2281/files ,
Ran only for horovod common to keep the PR manageable for review.

mengxr · 2021-10-06T18:16:46Z

@Vikas-kum We found an issue introduced by this commit. Our distributed filesystem doesn't support random writes (including re-open and append). It used to work fine with horovod timeline because it only create, writes the data sequentially, and then close. I wonder if we can keep the old behavior if users do not use the dynamic timeline feature. Any pointers on what we should change?

danzafar · 2021-10-11T17:27:12Z

Any next steps on this @Vikas-kum ?

Vikas-kum force-pushed the dynamic-timeline branch 3 times, most recently from 4a2daae to f927588 Compare August 29, 2020 01:00

Vikas-kum mentioned this pull request Aug 29, 2020

Added ability to dynamically start and stop timeline programmatically #2129

Closed

tgaddair reviewed Sep 1, 2020

View reviewed changes

Vikas-kum force-pushed the dynamic-timeline branch from 77fe6e2 to eb26a22 Compare September 3, 2020 22:40

tgaddair reviewed Sep 10, 2020

View reviewed changes

Vikas-kum force-pushed the dynamic-timeline branch 3 times, most recently from 6365cce to 896dca7 Compare September 12, 2020 02:02

Vikas-kum closed this Sep 12, 2020

Vikas-kum reopened this Sep 12, 2020

Vikas-kum force-pushed the dynamic-timeline branch 2 times, most recently from 3bb245f to 4613b7a Compare September 13, 2020 03:01

tgaddair and others added 12 commits September 12, 2020 20:02

Added ability to dynamically start and stop timeline programmatically

ca828da

Signed-off-by: Travis Addair <taddair@uber.com>

Added timeline API for all other frameworks

e1f3571

Signed-off-by: Travis Addair <taddair@uber.com>

Updated changelog

bff97e4

Signed-off-by: Travis Addair <taddair@uber.com>

Fixed resume timeline

0fdb06b

Signed-off-by: Travis Addair <taddair@uber.com>

Stop timeline

c525c3b

Signed-off-by: Travis Addair <taddair@uber.com>

Removed unused import

fea0d21

Signed-off-by: Travis Addair <taddair@uber.com>

Mutex

f11c7ec

Signed-off-by: Travis Addair <taddair@uber.com>

Changes to handles races for handling dynamic timeline update

a870742

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Adding start_time

afd8566

Adding mroe test Signed-off-by: Vikas Kumar <vikumar@amazon.com>

fix compile issue

ab11aa3

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

fix test

e12ff45

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

ran clang format

818bc30

remove debug statement addressed comments Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Vikas-kum added 3 commits September 12, 2020 20:02

clang-format

8148f83

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

temp

cd1bd1e

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

more checks

24cb454

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Vikas-kum force-pushed the dynamic-timeline branch from 4613b7a to 24cb454 Compare September 13, 2020 03:03

Vikas-kum added 4 commits September 12, 2020 21:24

refactor

26f54f9

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

logging

b7a0aeb

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

more logs

b34e5a5

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

test

90fb692

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Vikas-kum force-pushed the dynamic-timeline branch from 09a0832 to 90fb692 Compare September 14, 2020 16:22

Vikas-kum added 4 commits September 14, 2020 14:39

add to test

6d8ca48

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

check

0f8c234

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

ocompile error

8523b05

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

update tests

2889be9

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Vikas-kum force-pushed the dynamic-timeline branch from e42b5b5 to 2889be9 Compare September 14, 2020 23:37

Vikas-kum added 5 commits September 14, 2020 16:43

return if timeline is enabled

06624f5

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

stop_timeline

6459faa

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Add test

8c8b748

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

removing cout statements

c54a5c2

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

remove print from tests

d07b715

Signed-off-by: Vikas Kumar <vikumar@amazon.com>

Vikas-kum force-pushed the dynamic-timeline branch from 924bbdb to d07b715 Compare September 15, 2020 01:26

tgaddair approved these changes Sep 15, 2020

View reviewed changes

tgaddair merged commit a7de4ae into horovod:master Sep 15, 2020

tgaddair mentioned this pull request Sep 25, 2020

Fix race condition with dynamic timeline #2330

Open

romerojosh mentioned this pull request Apr 13, 2021

Unexpected performance decrease after hvd.init on CPU nodes #2804

Closed

romerojosh mentioned this pull request Apr 28, 2021

Require setting HOROVOD_TIMELINE=DYNAMIC to enable dynamic timeline API usage #2874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic timeline #2215

Dynamic timeline #2215

Vikas-kum commented Aug 28, 2020 •

edited

Loading

tgaddair commented Aug 29, 2020

tgaddair left a comment

tgaddair Sep 1, 2020

Vikas-kum Sep 3, 2020

Vikas-kum commented Sep 3, 2020

Vikas-kum commented Sep 9, 2020

tgaddair left a comment •

edited

Loading

Vikas-kum commented Sep 10, 2020 •

edited

Loading

tgaddair commented Sep 15, 2020

Vikas-kum commented Sep 15, 2020 •

edited

Loading

tgaddair left a comment

Vikas-kum commented Sep 15, 2020

mengxr commented Oct 6, 2021

danzafar commented Oct 11, 2021

Dynamic timeline #2215

Dynamic timeline #2215

Conversation

Vikas-kum commented Aug 28, 2020 • edited Loading

Checklist before submitting

Description

Review process to land

tgaddair commented Aug 29, 2020

tgaddair left a comment

Choose a reason for hiding this comment

tgaddair Sep 1, 2020

Choose a reason for hiding this comment

Vikas-kum Sep 3, 2020

Choose a reason for hiding this comment

Vikas-kum commented Sep 3, 2020

Vikas-kum commented Sep 9, 2020

tgaddair left a comment • edited Loading

Choose a reason for hiding this comment

Vikas-kum commented Sep 10, 2020 • edited Loading

tgaddair commented Sep 15, 2020

Vikas-kum commented Sep 15, 2020 • edited Loading

tgaddair left a comment

Choose a reason for hiding this comment

Vikas-kum commented Sep 15, 2020

mengxr commented Oct 6, 2021

danzafar commented Oct 11, 2021

Vikas-kum commented Aug 28, 2020 •

edited

Loading

tgaddair left a comment •

edited

Loading

Vikas-kum commented Sep 10, 2020 •

edited

Loading

Vikas-kum commented Sep 15, 2020 •

edited

Loading