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

Add sequence decoder integration tests #3175

Merged
merged 4 commits into from
Mar 3, 2023
Merged

Conversation

jeffkinnison
Copy link
Contributor

Adds integration tests for SequenceGeneratorDecoder in response to #3152.

@jeffkinnison jeffkinnison added the release-0.7 Needs cherry-pick into 0.7 release branch label Mar 1, 2023
@jeffkinnison
Copy link
Contributor Author

@tgaddair The accuracy discrepancy I mentioned earlier caused the distributed test failures here. It looks like it's coming from the eval vs local comparison in tests.integration_tests.utils.train_with_backend.

@tgaddair
Copy link
Collaborator

tgaddair commented Mar 1, 2023

@jeffkinnison that's what I figured. What this means is the distributed aggregation of the token_accuracy metric is not implemented correctly. We should probably fix that! Or if it's too much work, add the metric to the excluded list of metrics to compare in the test utils:

https://github.com/ludwig-ai/ludwig/blob/master/tests/integration_tests/utils.py#L908

@tgaddair
Copy link
Collaborator

tgaddair commented Mar 1, 2023

Also, if you're going to cherry-pick this manually with the other fix, I will remove the release-0.7 label so it doesn't get auto-picked into the branch.

@tgaddair tgaddair removed the release-0.7 Needs cherry-pick into 0.7 release branch label Mar 1, 2023
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

Unit Test Results

         6 files  ±  0           6 suites  ±0   5h 42m 15s ⏱️ - 24m 25s
  4 010 tests +29    3 966 ✔️ +29    44 💤 ±0  0 ±0 
12 051 runs  +87  11 910 ✔️ +87  141 💤 ±0  0 ±0 

Results for commit f30bd50. ± Comparison against base commit 4f6d6ae.

♻️ This comment has been updated with latest results.

@jeffkinnison jeffkinnison marked this pull request as ready for review March 3, 2023 02:41
@tgaddair tgaddair merged commit efd9337 into master Mar 3, 2023
@tgaddair tgaddair deleted the seq-gen-decoder-pred-test branch March 3, 2023 07:33
tgaddair pushed a commit that referenced this pull request Mar 3, 2023
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.

None yet

2 participants