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

Bug fix - test_csvs argument was ignored #2994

Merged
merged 1 commit into from
May 15, 2020

Conversation

Jendker
Copy link
Contributor

@Jendker Jendker commented May 15, 2020

Removed spurious overwriting of argument 'test_csvs' in evaluate.py
This bug leads to problems in lm_optimizer.py

Removed spurious overwriting of argument 'test_csvs' in evaluate.py
This bug lead to problems in lm_optimizer.py
@community-tc-integration
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

Copy link
Contributor

@kdavis-mozilla kdavis-mozilla left a comment

Choose a reason for hiding this comment

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

It's used on the very next line.

@Jendker
Copy link
Contributor Author

Jendker commented May 15, 2020

The variable is used on the next line, that's fine.

But the function argument has the same variable name and is overwritten.
This leads to problems in lm_optimizer.py where each time all test_csvs are used, because given argument is ignored.

@Jendker Jendker changed the title Bug fix - test_csvs argument was not used Bug fix - test_csvs argument was ignored May 15, 2020
@kdavis-mozilla
Copy link
Contributor

Have you checked all other calls to evaluate() to make sure they don't rely on this overwriting?

@Jendker
Copy link
Contributor Author

Jendker commented May 15, 2020

Yes, I've checked it. Everywhere else as argument FLAGS.test_files.split(',') was given, so the bug was hidden. Other case is only in lm_optimizer.py, where this fix corrects the pruner behaviour.

@kdavis-mozilla
Copy link
Contributor

Hmm.

Looks like the only other calls are train.py:643, evaluate.py:143, and lm_optimizer.py:36. All of those look like they don't rely on the overwriting and the only one which breaks as a result of the overwriting is lm_optimizer.py:36.

So you're change looks good.

Copy link
Contributor

@kdavis-mozilla kdavis-mozilla left a comment

Choose a reason for hiding this comment

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

LGTM

@kdavis-mozilla kdavis-mozilla merged commit 3b4c39f into mozilla:master May 15, 2020
@Jendker Jendker deleted the patch-1 branch May 15, 2020 17:34
@kdavis-mozilla
Copy link
Contributor

Thanks!

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.

2 participants