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.

Sign up for GitHub

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

Jump to bottom

Fixing strided perplexity calculation for fixed-length models #34394

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Fixing strided perplexity calculation for fixed-length models #34394

wants to merge 39 commits into from

Conversation

forrestdavis
Copy link

What does this PR do?

Fixes an issue with perplexity of fixed length models with a strided window. The perplexity calculation assumes all batches are the same size. This calculates the average with different context sizes. The update is just to perplexity.md in the docs.

Fixes #34138

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@ArthurZucker

@forrestdavis forrestdavis mentioned this pull request Oct 24, 2024
Open
4 tasks
yonigozlan and others added 26 commits October 24, 2024 20:00
@yonigozlan
Use non nested images and batched text Idefics2/3 (huggingface#34222)
940a6bd 
* add support for non nested images and add tests

* add tests error scenario

* fix style

* added single and no image to error tests
@IlyasMoutawwakil
Fix onnx non-expotable inplace aten op (huggingface#34376)
5779bac 
* fix onnx non-expotable inplace op

* mistral, qwen2, qwen2_vl, starcoder2

* fixup copies
@zucchini-nlp
Fix right padding in LLaVA models (huggingface#34305)
9f365fe 
* fix right pad llavas

* device mismatch
@ydshieh
no filter (huggingface#34391)
2238553 
* no filter

* no filter

* no filter

---------

Co-authored-by: ydshieh <[email protected]>
@gante
SynthID: better example (huggingface#34372)
8814043 
* better example

* Update src/transformers/generation/configuration_utils.py

* Update src/transformers/generation/logits_process.py

* nits
@gante
Tests: upgrade test_eager_matches_sdpa_generate (huggingface#34386)
186b8dc
@matthewdouglas
Fix bnb training test failure (huggingface#34414)
e447185 
* Fix bnb training test: compatibility with OPTSdpaAttention
@ydshieh
Avoid check expected exception when it is on CUDA (huggingface#34408)
f73f5e6 
* update

* update

---------

Co-authored-by: ydshieh <[email protected]>
@rudydel
Fix typos in agents_advanced.md (huggingface#34405)
6a62a6d
@stevhliu
[docs] Cache implementations (huggingface#34325)
1d06379 
cache
@IlyasMoutawwakil
Fix pix2struct (huggingface#34374)
fddbd3c 
* fix

* fix and test use_cache test

* style

* remove atol
@ydshieh
pin tensorflow_probability<0.22 in docker files (huggingface#34381)
fc465bb 
0.21

Co-authored-by: ydshieh <[email protected]>
@ydshieh
Tiny update after huggingface#34383 (huggingface#34404)
9360f18 
* update

* update

* update

---------

Co-authored-by: ydshieh <[email protected]>
@zeus2611 @muellerzr
Fix batch size handling in prediction_loop for DataLoaderShard (huggi…
92bcdff 
…ngface#34343)

* Fix batch size handling in prediction_loop for DataLoaderShard

Updated the prediction_loop method in the Trainer class to correctly handle batch size when using DataLoaderShard. This ensures that the batch size is retrieved from total_batch_size for distributed training scenarios, preventing TypeError related to NoneType during evaluation.

* Update src/transformers/trainer.py

Co-authored-by: Zach Mueller <[email protected]>

* Applied the fix to remove unused imports

---------

Co-authored-by: Zach Mueller <[email protected]>
@eljandoubi
exclude fsdp from delay_optimizer_creation (huggingface#34140)
8b3b9b4 
* exclude fsdp from delay_optimizer_creation

* add test case for trainer: FSDP mode and fp8 as mixed precision

* rearrange imports

* ruff formatted

* adapt _init_fsdp to fp8

* use _init_fsdp only when resume_from_checkpoint

* In case of FDP, self.layer will be CheckpointWrapper which has no len() method

* delete _init_fsdp

* solve conflict

* fix conflict

* make fixup
@seanswyi @ArthurZucker @muellerzr
New option called "best" for args.save_strategy. (huggingface#31817)
c175343 
* Add _determine_best_metric and new saving logic.

1. Logic to determine the best logic was separated out from
`_save_checkpoint`.
2. In `_maybe_log_save_evaluate`, whether or not a new best metric was
achieved is determined after each evaluation, and if the save strategy
is "best' then the TrainerControl is updated accordingly.

* Added SaveStrategy.

Same as IntervalStrategy, but with a new attribute called BEST.

* IntervalStrategy -> SaveStrategy

* IntervalStratgy -> SaveStrategy for save_strat.

* Interval -> Save in docstring.

* Updated docstring for save_strategy.

* Added SaveStrategy and made according changes.

`save_strategy` previously followed `IntervalStrategy` but now follows
`SaveStrategy`.

Changes were made accordingly to the code and the docstring.

* Changes from `make fixup`.

* Removed redundant metrics argument.

* Added new test_save_best_checkpoint test.

1. Checks for both cases where `metric_for_best_model` is explicitly
provided and when it's not provided.
2. The first case should have two checkpoints saved, whereas the second
should have three saved.

* Changed should_training_end saving logic.

The Trainer saves a checkpoints at the end of training by default as
long as `save_strategy != SaveStrategy.NO`. This condition was modified
to include `SaveStrategy.BEST` because it would be counterintuitive that
we'd only want the best checkpoint to be saved but the last one is as
well.

* `args.metric_for_best_model` default to loss.

* Undo metric_for_best_model update.

* Remove checking metric_for_best_model.

* Added test cases for loss and no metric.

* Added error for metric and changed default best_metric.

* Removed unused import.

* `new_best_metric` -> `is_new_best_metric`

Co-authored-by: Arthur <[email protected]>

* Applied `is_new_best_metric` to all.

Changes were made for consistency and also to fix a potential bug.

---------

Co-authored-by: Arthur <[email protected]>
Co-authored-by: Zach Mueller <[email protected]>
@h3110Fr13nd
[docs] update input documentation for MAMBA2 and MISTRAL models to in…
fc1ae7f 
…clude cache_position and attention_mask details (huggingface#34322)

* [docs] update input documentation for MAMBA2 and MISTRAL models to include cache_position and attention_mask details

* [docs] correct input documentation for MISTRAL model to reference `input_ids` instead of `decoder_input_ids`

* [docs] clarify cache_position description in MISTRAL model documentation
@Jwaminju @stevhliu
🌐 [i18n-KO] Translated model_doc/barthez.md to Korean (huggingface#…
1f7539c 
…33980)

* docs: ko: model_doc/barthez.md

* feat: nmt draft

---------

Co-authored-by: Steven Liu <[email protected]>
@ShubhamJagtap2000
Apply linting to the important code blocks to make it readable (huggi…
084e946 
…ngface#34449)

Enhance user experience using py-linting
@AhmedAlmaghz @abodacs
[i18n-ar] Translated file : docs/source/ar/fast_tokenizers.md into …
a17f287 
…Arabic (huggingface#33034)

* Add docs/source/ar/fast_tokenizers.md to Add_docs_source_ar_fast_tokenizers.md

* Update _toctree.yml

* Update _toctree.yml

* Update docs/source/ar/_toctree.yml

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

* Update docs/source/ar/fast_tokenizers.md

Co-authored-by: Abdullah Mohammed <[email protected]>

---------

Co-authored-by: Abdullah Mohammed <[email protected]>
@techkang @muellerzr @ArthurZucker
enable average tokens across devices (huggingface#34373)
d21dbd1 
* enable average tokens across devices

* reduce earlier in case model needs it

* simplify if statement

* reformat code to make ruff happy

* add doc for argument: average_tokens_across_devices

* cannot find world size when pytorch is unavailable

* format code

---------

Co-authored-by: Zach Mueller <[email protected]>
Co-authored-by: Arthur <[email protected]>
@McPatate
feat: run benchmarks on A100 (huggingface#34287)
6cc4a67
@alex-bene @ArthurZucker
Add post_process_depth_estimation for GLPN (huggingface#34413)
a769ed4 
* add depth postprocessing for GLPN

* remove previous temp fix for glpn tests

* Style changes for GLPN's `post_process_depth_estimation`

Co-authored-by: Arthur <[email protected]>

* additional style fix

---------

Co-authored-by: Arthur <[email protected]>
@zucchini-nlp
LLaVA: latency issues (huggingface#34460)
fe76b60 
* fix llavas

* code style

* green ci
@zucchini-nlp
Generation: fix test (huggingface#34369)
808d6c5 
* fix test

* fix copies
@zucchini-nlp
Fix CI (huggingface#34458)
63ca6d9 
* fix

* fix mistral
techkang and others added 4 commits October 29, 2024 09:39
@techkang
use a tinymodel to test generation config which aviod timeout (huggin…
655bec2 
…gface#34482)

* use a tinymodel to test generation config which aviod timeout

* remove tailing whitespace
@sbucaille @NielsRogge @qubvel
🚨🚨🚨 [SuperPoint] Fix keypoint coordinate output and add post processi…
a183519 
…ng (huggingface#33200)

* feat: Added int conversion and unwrapping

* test: added tests for post_process_keypoint_detection of SuperPointImageProcessor

* docs: changed docs to include post_process_keypoint_detection method and switched from opencv to matplotlib

* test: changed test to not depend on SuperPointModel forward

* test: added missing require_torch decorator

* docs: changed pyplot parameters for the keypoints to be more visible in the example

* tests: changed import torch location to make test_flax and test_tf

* Revert "tests: changed import torch location to make test_flax and test_tf"

This reverts commit 39b32a2.

* tests: fixed import

* chore: applied suggestions from code review

Co-authored-by: NielsRogge <[email protected]>

* tests: fixed import

* tests: fixed import (bis)

* tests: fixed import (ter)

* feat: added choice of type for target_size and changed tests accordingly

* docs: updated code snippet to reflect the addition of target size type choice in post process method

* tests: fixed imports (...)

* tests: fixed imports (...)

* style: formatting file

* docs: fixed typo from image[0] to image.size[0]

* docs: added output image and fixed some tests

* Update docs/source/en/model_doc/superpoint.md

Co-authored-by: Pavel Iakubovskii <[email protected]>

* fix: included SuperPointKeypointDescriptionOutput in TYPE_CHECKING if statement and changed tests results to reflect changes to SuperPoint from absolute keypoints coordinates to relative

* docs: changed SuperPoint's docs to print output instead of just accessing

* style: applied make style

* docs: added missing output type and precision in docstring of post_process_keypoint_detection

* perf: deleted loop to perform keypoint conversion in one statement

* fix: moved keypoint conversion at the end of model forward

* docs: changed SuperPointInterestPointDecoder to SuperPointKeypointDecoder class name and added relative (x, y) coordinates information to its method

* fix: changed type hint

* refactor: removed unnecessary brackets

* revert: SuperPointKeypointDecoder to SuperPointInterestPointDecoder

* Update docs/source/en/model_doc/superpoint.md

Co-authored-by: Pavel Iakubovskii <[email protected]>

---------

Co-authored-by: Steven Bucaille <[email protected]>
Co-authored-by: NielsRogge <[email protected]>
Co-authored-by: Pavel Iakubovskii <[email protected]>
@ydshieh
Simplify running tests in a subprocess (huggingface#34213)
439334c 
* check

* check

* check

* check

* add docstring

---------

Co-authored-by: ydshieh <[email protected]>
@Framartin
Fix perplexity computation in perplexity.md (huggingface#34387)
626c610 
fix average NLL in perplexity.md
ArthurZucker
ArthurZucker approved these changes Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks for updating

@ArthurZucker
Copy link
Collaborator

Could you resolve conflicts with main? 🤗

hlky and others added 6 commits October 29, 2024 11:40
@hlky @ArthurZucker
Fixes for Modular Converter on Windows (huggingface#34266)
9e3d704 
* Separator in regex

* Standardize separator for relative path in auto generated message

* open() encoding

* Replace `\` on `os.path.abspath`

---------

Co-authored-by: Arthur <[email protected]>
@SunMarc
Fix regression loading dtype (huggingface#34409)
004530a 
* fix regression

* add test for torchao

* expected output

* better fix
@forrestdavis
Fixing perplexity calculation
5235c6c
@forrestdavis
Adding back in tqdm
0cefb99
@forrestdavis
Adding back in a space from before
c97221d 
Fix here is simpler
@forrestdavis
Removing excess parentheses
fedc243
@forrestdavis
Copy link
Author

Thanks! I see that there were changes to address this. If those are preferred that's fine with me. I think the proposal here is simpler, but that's subjective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurZucker ArthurZucker ArthurZucker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect average calculation in Perplexity of fixed-length models