Skip to content

Conversation

@ykumards
Copy link
Contributor

@ykumards ykumards commented Jan 26, 2020

Fixes #589

Description:

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@ykumards
Copy link
Contributor Author

ykumards commented Jan 26, 2020

Hello @vfdev-5,

I picked off from PR #604. I have two questions/problems before we can finalize this:

  1. I am not sure how to come up with test cases for the data_flow_timers. If I understand the code correctly, they are timed between the Events.STARTED and the first Events.ITERATION_STARTED. Does this mean the delay has to be tested in the dataloader function? Am not really sure how to go about this.

  2. I found that there are two other events (Events.GET_BATCH_COMPLETED, Events.GET_BATCH_STARTED) that is not handled in the profiler code. Is this fine? I just added them to events_to_ignore for now.

@ykumards ykumards changed the title Issue 589 Issue #589 Jan 26, 2020
@ykumards ykumards changed the title Issue #589 Issue 589 Jan 26, 2020
@ykumards ykumards changed the title Issue 589 Issue #589: Add basic time profiler to contrib.handlers Jan 26, 2020
@ykumards ykumards changed the title Issue #589: Add basic time profiler to contrib.handlers Issue 589: Add basic time profiler to contrib.handlers Jan 26, 2020
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 26, 2020

@ykumards thanks for the PR

I start with the second question:

I found that there are two other events (Events.GET_BATCH_COMPLETED, Events.GET_BATCH_STARTED) that is not handled in the profiler code. Is this fine? I just added them to events_to_ignore for now.

These events were introduced very recently, much after BasicTimeProfiler, so yes we need to adapt the class for these events. I think even that they would simlify some of actual logic of BasicTimeProfiler.

I am not sure how to come up with test cases for the data_flow_timers. If I understand the code correctly, they are timed between the Events.STARTED and the first Events.ITERATION_STARTED. Does this mean the delay has to be tested in the dataloader function? Am not really sure how to go about this.

Yes, idea is to provide some measures on dataflow preparation: dataflow times vs forward/backward passes times.
To test this you can setup a data iterator, something like this

import time

def data_iterator(data):
    while True:
        for d in data:
            time.sleep(0.01)
            yield d

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 2, 2020

@ykumards thanks! So, it remains a dump to CSV and we are done :)

@vfdev-5 vfdev-5 marked this pull request as ready for review February 2, 2020 20:34
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

@ykumards, we are almost perfect :) Just some minor things to fix.

We also need to update the documentation: https://github.com/pytorch/ignite/blob/master/docs/source/contrib/handlers.rst

You can for example add a section after "param_scheduler"

time_profilers
---------------

.. automodule:: ignite.contrib.handlers.time_profilers
   :members:

None

Attributes:
data_flow_times (torch.Tensor): tile elapsed during data loading
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
data_flow_times (torch.Tensor): tile elapsed during data loading
data_flow_times (torch.Tensor): time elapsed during data loading

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 2, 2020

And, for instance, let's do not pay attention to codecov/patch

>>> profiler.print_results(results)

The results would look as follows:
--------------------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ykumards I think we need to insert the output inside

    .. code-block:: text

        - Time profiling results:
        --------------------------------------------
        Processing function time stats (in seconds):
            min/index: (2.9754010029137135e-05, 0)
            max/index: (2.9754010029137135e-05, 0)
            mean: 2.9754010029137135e-05
            std: nan
            total: 2.9754010029137135e-05

event type, processing stats and dataflow stats and may look as
follows:

--------------------------------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, please use

    .. code-block:: text

        --------------------------------------------
        epoch iteration processing_stats dataflow_stats Event_STARTED ...

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 3, 2020

@ykumards minor fixes still needed i think. You can check documentation rendering using:

cd docs && pip install -r requirements.txt && make html
cd build/html && python -m http.server 1234

In the browser open 0.0.0.0:1234 and it should be there.

Thank you.

@ykumards
Copy link
Contributor Author

ykumards commented Feb 4, 2020

Hello @vfdev-5

Thanks! The docstring looks better formatted now. I moved the code and output examples to the corresponding function definitions, hoe that's okay.

Screenshots attached below:

Screen Shot 2020-02-04 at 10 48 53 AM
Screen Shot 2020-02-04 at 10 49 00 AM

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Yogesh!

@ykumards
Copy link
Contributor Author

ykumards commented Feb 4, 2020

LGTM! Thanks Yogesh!

Just made one tiny change to fix these guys :D

Screen Shot 2020-02-04 at 10 49 00 AM

@vfdev-5 vfdev-5 merged commit 0e97cb2 into pytorch:master Feb 4, 2020
@ykumards ykumards deleted the issue-589 branch February 4, 2020 10:58
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.

Basic Time Profiling into contrib

2 participants