facebookresearch / facebookresearch/detectron2

max_to_keep unused in PeriodicCheckpointer class.

Open
#1,803 3 comments 0 reactions 0 assignees View on GitHub
contributions welcome enhancement
Dominant language
Python
Stars
34.7k
Forks
7.9k
PR merge metrics
No merged PRs in 30d

Description

## 🚀 Feature

The class `PeriodicCheckpointer` (in `hooks.py`) inherits from class of the same name in `fvcore` library, i.e., `from fvcore.common.checkpoint import PeriodicCheckpointer as _PeriodicCheckpointer` in `hooks.py`.

The parent class has the instance parameter `max_to_keep` which defines how many checkpoints to keep.

The child class is instantiated in `DefaultTrainer` in the `build_hooks` member function of the `DefaultTrainer` class. However, it is done so without specifying `max_to_keep`. Therefore, it is not possible to specify this parameter without overwriting the entire `build_hooks` function, which is quite large.

Would it be possible to add the initialisation of this parameter into `build_hooks`?

Also, as an aside, `max_iter` is instantiated in a strange manner in the child version of the `PeriodicCheckpointer` class. In particular, it is set within the `before_train` function. Why is it done in this manner and not directly in the `build_hooks` function?

## Motivation & Examples

It would be useful because I would not like to maintain the whole history of checkpoints, which can be large and take up a lot of space. Also, when using tools such as mlflow to log models, it will reduce the amount of checkpoints that have to be logged to the tracking server.

Describe what the feature would look like, if it is implemented.
Best demonstrated using **code examples** in addition to words.

I believe the change would be simple. Something along the lines of the following updated line, along with an appropriate default setting of cfg.SOLVER.MAX_TO_KEEP to maintain backwards compatibility.

`
if comm.is_main_process():
ret.append(hooks.PeriodicCheckpointer(self.checkpointer, cfg.SOLVER.CHECKPOINT_PERIOD, cfg.SOLVER.MAX_ITER, cfg.SOLVER.MAX_TO_KEEP))
`

I am happy to implement any changes that are agreed upon.

## Note

We only consider adding new features if they are relevant to many users.

If you request implementation of research papers -- we only consider papers that have enough significance and prevalance in the object detection field.

We do not take requests for most projects in the `projects/` directory, because they are research code release that is mainly for other researchers to reproduce results.

"Make X faster/accurate" is not a valid feature request. "Implement a concrete feature that can make X faster/accurate" can be a valid feature request.

Instead of adding features inside detectron2,
you can implement many features by [extending detectron2](https://detectron2.readthedocs.io/tutorials/extend.html).
The [projects/](https://github.com/facebookresearch/detectron2/tree/master/projects/) directory contains many of such examples.

Contributor guide

Open the contributing guide

Research direction

Start in hooks.py, especially the PeriodicCheckpointer class and DefaultTrainer.build_hooks, then compare their initialization with fvcore's parent class. Trace how cfg.SOLVER.CHECKPOINT_PERIOD and cfg.SOLVER.MAX_ITER are passed, and determine the appropriate default for a new max_to_keep setting. Done means the setting can be configured without overriding build_hooks while preserving existing behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
machine-learning
Issue type
Feature
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.