Added max/min number of steps in Trainer

[peteriz]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
• Did you read the contributor guideline?
• Did you make sure to update the docs?
• Did you write any new necessary tests?

What does this PR do?

Partially Fixes #640
I added a max_steps argument in Trainer for stopping the training process when max_steps has been reached. This feature is highly desired in Transformer-based models (such as BERT, XLNet, etc.) where a warm-up phase and LR decay is required.
I will add future PRs covering stepwise processing (related to scheduler)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

[williamFalcon]

@peteriz good addition. please update docs and add a test

[peteriz]

@williamFalcon @Borda
All comments were addressed.

[williamFalcon]

@peteriz awesome! mind adding a test for this?
Trainer is something we're super strict with testing.

[peteriz]

No problem.
Can you direct me to similar test? I didn't find a similar test for epochs

[williamFalcon]

A test for this would run trainer with all 4 of those cases. So, min_steps, max_steps and then make sure the step counts are correct.

edit:
just looked, looks like we don't directly test for that, a lot of tests do limit the epochs though. Maybe this is a good time to add it? It would be for this file:

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/test_trainer.py

To make sure the test is fast, maybe set a limit on how much training data is used? 10%?

[peteriz]

The intention of my question was on where to place the test.
Would tests/test_trainer.py be the right place?

[williamFalcon]

yup! edited my original comment

[peteriz]

@williamFalcon I pushed the test, I hope it is okay.

[williamFalcon]

Failed on GPU:

______________ test_num_trainer_steps ________________________________________

tmpdir = local('/tmp/pytest-of-waf251/pytest-5/test_num_trainer_steps0')

    def test_num_trainer_steps(tmpdir):
        """Verify model trains according to speficied steps"""
        tutils.reset_seed()
        model, _ = tutils.get_model()

        trainer_options = dict(
            max_epochs=5,
            gpus=None,
            default_save_path=tmpdir,
            train_percent_check=0.05,
        )

        trainer_options['max_epochs'] = 2
        trainer_options['max_steps'] = 100
        trainer = Trainer(**trainer_options)
        result = trainer.fit(model)
        assert result == 1
        # should stop at max_steps
        assert trainer.global_step == 100, "Model did not stop at max_steps"

        trainer_options['max_epochs'] = 2
        trainer_options['max_steps'] = 500
        trainer = Trainer(**trainer_options)
        result = trainer.fit(model)
        assert result == 1
        # should stop at max_epochs
>       assert trainer.global_step == 93 * 2 and \
            trainer.current_epoch == 1, "Model did not stop at max_epochs"
E       AssertionError: Model did not stop at max_epochs
E       assert (278 == 186
E         -278
E         +186)

[peteriz]

Thought Travis-CI pass was okay. I'll have a look.
I dont get why it failed GPU, the trainer is configured with gpus=None

[Borda]

unfortunately, it is hard to get free CI with GPUs for full testing, but we are working on it.. #486

[williamFalcon]

still failing:

tmpdir = local('/tmp/pytest-of-waf251/pytest-6/test_num_trainer_steps0')

    def test_num_trainer_steps(tmpdir):
        """Verify model trains according to speficied steps"""
        tutils.reset_seed()
        model, _ = tutils.get_model()

        trainer_options = dict(
            max_epochs=5,
            gpus=None,
            default_save_path=tmpdir,
            train_percent_check=0.05,
        )

        trainer_options['max_epochs'] = 2
        trainer_options['max_steps'] = 100
        trainer = Trainer(**trainer_options)
        result = trainer.fit(model)
        assert result == 1
        # should stop at max_steps
        assert trainer.global_step == 100, "Model did not stop at max_steps"

        trainer_options['max_epochs'] = 2
        trainer_options['max_steps'] = 500
        trainer = Trainer(**trainer_options)
        result = trainer.fit(model)
        assert result == 1
        # should stop at max_epochs
>       assert trainer.global_step == 93 * 2 and \
            trainer.current_epoch == 1, "Model did not stop at max_epochs"
E       AssertionError: Model did not stop at max_epochs
E       assert (278 == 186
E         -278
E         +186)

[peteriz]

@williamFalcon I could not reproduce that error on my GPU machine.
I changed the test to pull train set size from the data loader. Fingers crossed this should pass.

[williamFalcon]

@peteriz can you rebase master so we can see if tests pass now?

[peteriz]

Fixed. Could you squash my commits when merging?
Thanks

[Borda]

Fixed. Could you squash my commits when merging?

sure, that's what we always do... :]

[Borda]

Almost there, just a minor points, pls :] Thx for you patience...

[pep8speaks]

Hello @peteriz! Thanks for updating this PR.

• In the file pytorch_lightning/trainer/trainer.py:

Line 357:101: E501 line too long (106 > 100 characters)

Comment last updated at 2020-02-18 13:11:53 UTC

[Borda]

Hello @peteriz! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

the config is waiting here - #842 but he is right, the test is failing there...

[williamFalcon]

>       assert trainer.global_step == num_train_samples * trainer_options['max_epochs']
E       assert 278 == 186
E         -278
E         +186

Not sure what's happening here. Likely, the global steps are ACTUALLY wrong or perhaps this isn't accounting for batch size in your num_train_samples * trainer_options['max_epochs'] calculation?

[peteriz]

@williamFalcon Okay I changed the assert and it pulls max_epochs from the trainer.
I think this assert is essential for validating that the trainer stopped number_of_epochs * steps_per_epoch.

[williamFalcon]

@peteriz this is still failing...

>       assert result == 1 and trainer.global_step == num_train_samples * trainer.max_nb_epochs \
            and trainer.current_epoch == trainer.max_nb_epochs - 1, "Model did not stop at max_epochs"
E       AssertionError: Model did not stop at max_epochs
E       assert (1 == 1
E         -1
E         +1 and 278 == 186
E         -278
E         +186)```

I'd also say that nested asserts are hard to debug. My suggestion is to split this into two asserts, and use PDB to figure out what's going on.

I'd love to help finish this but I'm pretty deep into another feature so I don't have the bandwith to debug this.

I'd also say to run this on 2 GPUs... not sure why this is different.

[peteriz]

@williamFalcon I can separate the asserts back, but I was not able to get this error on my mac (python 3.7) or on my machines, cpu backed or 1/2/4 GPUs.
I need more info on what you're running (beside the test and CI here) ..

[peteriz]

@Borda @williamFalcon What can we do to push this PR forward? It would be great to make this happen since I'm planning on integrating pytorch-lightning into our repo NLP Architect.

[williamFalcon]

agreed! this is a critical PR for us.
But we reaaally need this test to pass. I think something is wrong with the functionality... there should be no caveats or what ifs on where these tests pass. if they don’t pass as they are on the machines i test on then we need to fix the functionality or test :)

so, maybe think about what would cause the difference in machines? and make sure you’re running the tests correctly.

i do bash .run_local_tests.sh on a 2 gpu machine

[williamFalcon]

maybe it’s somehow loading a different checkpoint?? check the file cache clearing

[williamFalcon]

@neggert

[williamFalcon]

ok, merged. @Borda mentioned this passed on his machine. we can dig into it deeper if it causes problems.

great addition!

[peteriz]

@williamFalcon @Borda Thanks for reviewing. Expect more NLP model related contributions 🚀