Skip to content

fix(eval): no need to check length of valid_dl when using streaming dataset#274

Merged
yhcc merged 3 commits into
InternLM:mainfrom
00INDEX:main
Sep 4, 2023
Merged

fix(eval): no need to check length of valid_dl when using streaming dataset#274
yhcc merged 3 commits into
InternLM:mainfrom
00INDEX:main

Conversation

@00INDEX

@00INDEX 00INDEX commented Sep 4, 2023

Copy link
Copy Markdown
Contributor

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

image

As shown in the figure above, when StreamingDataset is enabled, evaluator still tries to determine whether the dataset is empty by the dataset length. This is completely unnecessary, first of all, StreamingDataset does not have the len method, and secondly, StreamingDataset filters out empty datasets during the construction process.

Modification

So I modified the judgment condition to not judge the length when StreamingDataset is enabled

BC-breaking (Optional)

Does the modification introduce changes that break the backward compatibility of the downstream repositories?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here and update the documentation.

Checklist

Before PR:

  • Pre-commit or other linting tools are used to fix the potential lint issues.
  • Bug fixes are fully covered by unit tests, the case that causes the bug should be added in the unit tests.
  • The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, like docstring or example tutorials.

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with those projects.
  • CLA has been signed and all committers have signed the CLA in this PR.

@yhcc yhcc merged commit 5238f15 into InternLM:main Sep 4, 2023
@sunpengsdu sunpengsdu added the Bug Something isn't working label Sep 7, 2023
@sunpengsdu sunpengsdu added this to the v0.2.1dev20230908 milestone Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants