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

Various args and test fix #1909

Merged
merged 19 commits into from
Aug 9, 2024
Merged

Various args and test fix #1909

merged 19 commits into from
Aug 9, 2024

Conversation

qgallouedec
Copy link
Member

@qgallouedec qgallouedec commented Aug 7, 2024

Some test doesn't pass locally. Consequently, I propose the following fix/improvement:

1. Remove unused peft_test marker

tests/test_ppo_trainer.py:910
  /fsx/qgallouedec/trl/tests/test_ppo_trainer.py:910: PytestUnknownMarkWarning: Unknown pytest.mark.peft_test - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @mark.peft_test

2. Don't report to wandb

wandb does not cope well with multiple executions in the same process, and generates errors such as

wandb.sdk.lib.config_util.ConfigError: Attempted to change value of key "model/num_parameters" from 1658681 to 1663801

the easiest is not to report during test. Already fixed for reward tester in #1895

3. Don't share model across tests

Also partially addressed in #1895. When sharing the model, it creates conflicts between tests, due to model re-training mostly.

FAILED tests/test_dpo_trainer.py::DPOTrainerTester::test_dpo_trainer_w_dataset_num_proc - _pickle.PicklingError: Cannot pickle a prepared model with automatic mixed precision, please unwrap the model with `Accelerator.unwrap_model(model)` before pickling it.

The solution is to replace setUpClass by setUp (so that the setup is not shared)

4. Fixes configuration argument being overrided by deprecated default values

Fixes #1905 and adds tests.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@qgallouedec qgallouedec marked this pull request as ready for review August 7, 2024 12:07
@qgallouedec qgallouedec changed the title Various test fix Various args and test fix Aug 8, 2024
@qgallouedec qgallouedec requested a review from kashif August 9, 2024 08:04
@qgallouedec
Copy link
Member Author

Finally 😮‍💨

@qgallouedec qgallouedec linked an issue Aug 9, 2024 that may be closed by this pull request
@qgallouedec qgallouedec merged commit cbcaa46 into main Aug 9, 2024
10 checks passed
@qgallouedec qgallouedec deleted the various-test-fix branch August 9, 2024 08:08
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.

Configuration argument overriding by deprecated default values
3 participants