Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add declarative trainer #924

Closed
wants to merge 11 commits into from
Closed

Add declarative trainer #924

wants to merge 11 commits into from

Conversation

Minyus
Copy link

@Minyus Minyus commented Apr 15, 2020

Fixes # 912

Description:
Higher API for training will improve usability of Ignite.

Check list:

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

@sdesrozis
Copy link
Contributor

@Minyus Thank you very much for this PR !!

I make you my feedback on your proposal quickly :)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 15, 2020

@Minyus thanks a lot for the PR ! I really appreciate your contribution, however it would be great to discuss more about the API. We already stated some general remarks on the API in the related issue #912

In my opinion, class NetworkTrain has very specific integrations with MLflow which can be a limitation for some users. Another point is about exposing trainer in the higher API in case of adding more user-specific handlers.

please, let's discuss more about the API in #912.

@vfdev-5 vfdev-5 marked this pull request as draft April 17, 2020 13:19
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 17, 2020

@Minyus can we work out this at first, please ?

class TimeLimit:
    def __init__(self, limit_sec=3600):
        self.limit_sec = limit_sec
        self.start_time = time.time()

    def __call__(self, engine):
        elapsed_time = time.time() - self.start_time
        if elapsed_time > self.limit_sec:
            log.warning("Reached the time limit: {} sec. Stop training".format(self.limit_sec))
            engine.terminate()

This looks good enough. Maybe, in addition, we can set Checkpoint or ModelCheckpoint to save the work before terminating ?

Idea is to put it directly into ignite/handlers/time_limit.py

@Minyus
Copy link
Author

Minyus commented Apr 17, 2020

@vfdev-5
OK, I'll move TimeLimit to ignite/handlers/time_limit.py.
Let's add saving before terminating to ToDo list as it needs further consideration (e.g. what if model checkpoint path is not specified, etc.)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 17, 2020

@vfdev-5
OK, I'll move TimeLimit to ignite/handlers/time_limit.py.
Let's add saving before terminating to ToDo list as it needs further consideration (e.g. what if model checkpoint path is not specified, etc.)

@Minyus thanks, but it would be much better to split this PR into several ones. One PR by feature.
So, please, send the code of TimeLimit into a new PR. It would be nice to follow https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md
Thanks

@Minyus
Copy link
Author

Minyus commented Apr 19, 2020

As discussed in Slack, this PR is suspended.

Meanwhile, please feel free to send PR (including adding documentation, test, example, etc.) to PipelineX so that I can send a complete PR to Ignite later on.

higher API for training
https://github.com/Minyus/pipelinex/blob/master/src/pipelinex/ops/ignite/declaratives/declarative_trainer.py

MNIST example to use higher API
https://github.com/Minyus/pipelinex/blob/master/examples/mnist/mnist_with_declarative_trainer.py

TimeLimit handler
https://github.com/Minyus/pipelinex/blob/master/src/pipelinex/ops/ignite/handlers/time_limit.py

Cohen Kappa Score metric
https://github.com/Minyus/pipelinex/blob/master/src/pipelinex/ops/ignite/metrics/cohen_kappa_score.py

Please also feel free to copy any code in PipelineX to use in PyTorch Ignite.
I'm happy to see Ignite to be enhanced in any way.

@sdesrozis
Copy link
Contributor

Thank you again and see you soon !!

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.

3 participants