Key terms support #229

Enkidu93 · 2023-12-01T20:39:28Z

Minor fixes to support changes in the Machine PR. In general, if you could leave the parallel PRs open until both are approved, that might be helpful.

This change is

…ratext project test

codecov-commenter · 2023-12-01T20:45:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8c25097) 62.39% compared to head (62c192f) 62.39%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #229   +/-   ##
=======================================
  Coverage   62.39%   62.39%           
=======================================
  Files         134      134           
  Lines        5826     5826           
  Branches      912      912           
=======================================
  Hits         3635     3635           
  Misses       1803     1803           
  Partials      388      388

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ddaspit

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/data/TestProject.zip line 0 at r1 (raw file):
We don't want to check in binary files. Can you check in the contents, and create the zip file in the test?

Enkidu93 · 2023-12-01T20:50:48Z

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/data/TestProject.zip line 0 at r1 (raw file): We don't want to check in binary files. Can you check in the contents, and create the zip file in the test?

OK, there was already a zip, but I can do that if you like.

ddaspit

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/data/TestProject.zip line at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, there was already a zip, but I can do that if you like.

I should have caught it on the PR that added this file. Unfortunately, every time we make changes to this file it adds 2MB to our repo size. I would like to correct that sooner rather than later.

Enkidu93 · 2023-12-01T21:30:34Z

tests/Serval.E2ETests/data/TestProject.zip line at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I should have caught it on the PR that added this file. Unfortunately, every time we make changes to this file it adds 2MB to our repo size. I would like to correct that sooner rather than later.

Done.

johnml1135 · 2023-12-04T13:40:26Z

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 780 at r2 (raw file):

    /// A typical use case would be to set `"options"` to `{"max_steps":10}` in order to configure the maximum
    /// number of training iterations in order to reduce turnaround time for testing purposes. See [this](https://github.com/sillsdev/serval/wiki/Paratext-Key-Terms-Parsing)
    /// for using `"options"` to include Paratext key terms data in training.

It should be enabled by default - and clear from the documentation (it is not).

Enkidu93 · 2023-12-04T13:41:47Z

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 780 at r2 (raw file):

    /// A typical use case would be to set `"options"` to `{"max_steps":10}` in order to configure the maximum
    /// number of training iterations in order to reduce turnaround time for testing purposes. See [this](https://github.com/sillsdev/serval/wiki/Paratext-Key-Terms-Parsing)
    /// for using `"options"` to include Paratext key terms data in training.

It should be enabled by default - and clear from the documentation (it is not).

Yep, I'm just getting to that. Just switched it to default in Machine.

johnml1135 · 2023-12-04T13:45:36Z

tests/Serval.E2ETests/ServalApiTests.cs line 4 at r2 (raw file):

using System.Reflection.PortableExecutable;
using System.Runtime.InteropServices;
using DnsClient.Protocol;

If I am correct, all usings go into the global using document...

johnml1135 · 2023-12-04T13:47:00Z

tests/Serval.E2ETests/ServalApiTests.cs line 365 at r2 (raw file):

        await _helperClient!.ClearEngines();
        ZipFile.CreateFromDirectory("../../../data/TestProject", "TestProject.zip");
        ZipFile.CreateFromDirectory("../../../data/TestProjectTarget", "TestProjectTarget.zip");

I like how you zip up the files so that changes to USFM can be tracked in git!

johnml1135 · 2023-12-04T13:47:27Z

tests/Serval.E2ETests/ServalApiTests.cs line 365 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

I like how you zip up the files so that changes to USFM can be tracked in git!

That is, zip them up during the test and leave them uncompressed in the repo.

johnml1135 · 2023-12-04T13:48:20Z

tests/Serval.E2ETests/ServalApiTests.cs line 400 at r2 (raw file):

            new PretranslateCorpusConfig { CorpusId = corpus.Id, TextIds = new string[] { "JHN", "REV" } }
        );
        _helperClient.TranslationBuildConfig.Options = "{\"max_steps\":10, \"use_key_terms\":true}";

If it is added by default, why include it here? How can you test that it is actually being used?

johnml1135

Reviewed 2 of 5 files at r1, 25 of 25 files at r2, 1 of 3 files at r3.
Reviewable status: 26 of 28 files reviewed, 5 unresolved discussions (waiting on @ddaspit and @Enkidu93)

Enkidu93 · 2023-12-04T13:51:18Z

tests/Serval.E2ETests/ServalApiTests.cs line 400 at r2 (raw file):
            new PretranslateCorpusConfig { CorpusId = corpus.Id, TextIds = new string[] { "JHN", "REV" } }
        );
        _helperClient.TranslationBuildConfig.Options = "{\"max_steps\":10, \"use_key_terms\":true}";
If it is added by default, why include it here? How can you test that it is actually being used?

I can remove it if you like - it shouldn't make a difference. The purpose of this test isn't really to capture that functionality/logic - that's covered in the NmtPreprocessBuildJobTests. I figured leaving it explicit might be beneficial, and it's also more robust if we were to change the defaults.

Enkidu93

Reviewable status: 26 of 28 files reviewed, 5 unresolved discussions (waiting on @ddaspit and @johnml1135)

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 780 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yep, I'm just getting to that. Just switched it to default in Machine.

Done. Also updated the Wiki.

tests/Serval.E2ETests/ServalApiTests.cs line 4 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

If I am correct, all usings go into the global using document...

Done. These were mainly auto-usings added by VSCode inadvertently.

tests/Serval.E2ETests/ServalApiTests.cs line 365 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

That is, zip them up during the test and leave them uncompressed in the repo.

Yes, this is as per Damien's request - trackable. I also paired them down for the sake of space.

johnml1135 · 2023-12-04T18:23:11Z

tests/Serval.E2ETests/ServalApiTests.cs line 400 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I can remove it if you like - it shouldn't make a difference. The purpose of this test isn't really to capture that functionality/logic - that's covered in the NmtPreprocessBuildJobTests. I figured leaving it explicit might be beneficial, and it's also more robust if we were to change the defaults.

That's fine.

johnml1135

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

ddaspit

Reviewed 24 of 25 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 780 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. Also updated the Wiki.

It would probably be good to mention that we only include proper names.

tests/Serval.E2ETests/ServalApiTests.cs line 359 at r3 (raw file):

    {
        await _helperClient!.ClearEngines();
        ZipFile.CreateFromDirectory("../../../data/TestProject", "TestProject.zip");

We should wrap all of this in a try/finally so that we ensure that the zip files are deleted.

try
{
    ZipFile.CreateFromDirectory("../../../data/TestProject", "TestProject.zip");
    ZipFile.CreateFromDirectory("../../../data/TestProjectTarget", "TestProjectTarget.zip");
    // upload files
    ...
}
finally
{
    if (File.Exists("TestProject.zip"))
        File.Delete("TestProject.zip");
    if (File.Exists("TestProjectTarget.zip"))
        File.Delete("TestProjectTarget.zip");
}

It would also be good to create these files in a temp directory.

Enkidu93

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 780 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

It would probably be good to mention that we only include proper names.

Done.

tests/Serval.E2ETests/ServalApiTests.cs line 359 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should wrap all of this in a try/finally so that we ensure that the zip files are deleted.

try
{
    ZipFile.CreateFromDirectory("../../../data/TestProject", "TestProject.zip");
    ZipFile.CreateFromDirectory("../../../data/TestProjectTarget", "TestProjectTarget.zip");
    // upload files
    ...
}
finally
{
    if (File.Exists("TestProject.zip"))
        File.Delete("TestProject.zip");
    if (File.Exists("TestProjectTarget.zip"))
        File.Delete("TestProjectTarget.zip");
}

It would also be good to create these files in a temp directory.

Done.

ddaspit

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: 27 of 28 files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/ServalApiTests.cs line 359 at r3 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done.

I should have been more clear. I mean in the operating systems' temp folder. You can get it using Path.GetTempPath.

Enkidu93 · 2023-12-04T20:02:17Z

tests/Serval.E2ETests/ServalApiTests.cs line 359 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I should have been more clear. I mean in the operating systems' temp folder. You can get it using Path.GetTempPath.

No worries. It seemed odd to me. This makes more sense haha. Done.

ddaspit

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/ServalApiTests.cs line 364 at r5 (raw file):

        try
        {
            ZipFile.CreateFromDirectory("../../../data/TestProject", $"{tempDirectory}/TestProject.zip");

You should use Path.Combine to construct paths.

Enkidu93 · 2023-12-04T20:24:16Z

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/ServalApiTests.cs line 364 at r5 (raw file):
        try
        {
            ZipFile.CreateFromDirectory("../../../data/TestProject", $"{tempDirectory}/TestProject.zip");
You should use Path.Combine to construct paths.

For all paths? E.g., including "../../somedir/somefile.ext"? Or in regards to the tempDirectory? If you want it everywhere, I should change it a few other places as well.

ddaspit

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)

tests/Serval.E2ETests/ServalApiTests.cs line 364 at r5 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

For all paths? E.g., including "../../somedir/somefile.ext"? Or in regards to the tempDirectory? If you want it everywhere, I should change it a few other places as well.

Yes, you should use it everywhere.

Enkidu93

Reviewable status: 27 of 28 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)

tests/Serval.E2ETests/ServalApiTests.cs line 364 at r5 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yes, you should use it everywhere.

OK, done. In Machine, I've also changed it except in the uses of shared service OpenRead following your usage of OpenWrite in the build job.

johnml1135

Reviewed 1 of 2 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

ddaspit

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

This reverts commit 7ea2794.

This reverts commit f5fd2c9.

Enkidu93 added 4 commits November 28, 2023 15:25

Alter tests to accomodate key terms support and more fully cover a Pa…

9d3093f

…ratext project test

Added wiki, updated swagger, fixed test data

89017fd

Fixed test data

0440313

Fixes to test data

3274975

Enkidu93 requested a review from johnml1135 December 1, 2023 20:39

ddaspit requested changes Dec 1, 2023

View reviewed changes

ddaspit reviewed Dec 1, 2023

View reviewed changes

Enkidu93 added 2 commits December 1, 2023 16:16

Remove zips and include complete folders

832ebd4

Pair down project size

8e17036

Update docs to reflect changing defaults

d786c5f

Remove unecessary usings; move usings to global

faaf0a3

johnml1135 reviewed Dec 4, 2023

View reviewed changes

Enkidu93 commented Dec 4, 2023

View reviewed changes

johnml1135 approved these changes Dec 4, 2023

View reviewed changes

ddaspit reviewed Dec 4, 2023

View reviewed changes

PR review fixes

6ca7bad

Enkidu93 commented Dec 4, 2023

View reviewed changes

ddaspit requested a review from johnml1135 December 4, 2023 19:45

ddaspit reviewed Dec 4, 2023

View reviewed changes

More PR review changes

d5aa07c

ddaspit reviewed Dec 4, 2023

View reviewed changes

Use Path.Combine()

0a101fe

Enkidu93 commented Dec 4, 2023

View reviewed changes

johnml1135 approved these changes Dec 5, 2023

View reviewed changes

ddaspit approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'main' into key_terms_support

62c192f

Enkidu93 merged commit 7ea2794 into main Dec 6, 2023
1 check passed

Enkidu93 deleted the key_terms_support branch December 6, 2023 20:28

Enkidu93 added a commit that referenced this pull request Dec 11, 2023

Revert "Key terms support (#229)"

f5fd2c9

This reverts commit 7ea2794.

Enkidu93 added a commit that referenced this pull request Dec 11, 2023

Revert "Key terms support (#229)"

73b5e42

This reverts commit 7ea2794.

Enkidu93 added a commit that referenced this pull request Dec 11, 2023

Revert "Revert "Key terms support (#229)""

0d02024

This reverts commit f5fd2c9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Key terms support #229

Key terms support #229

Enkidu93 commented Dec 1, 2023 •

edited by ddaspit

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading

ddaspit left a comment

Enkidu93 commented Dec 1, 2023

ddaspit left a comment

Enkidu93 commented Dec 1, 2023

johnml1135 commented Dec 4, 2023

Enkidu93 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 left a comment

Enkidu93 commented Dec 4, 2023

Enkidu93 left a comment

johnml1135 commented Dec 4, 2023

johnml1135 left a comment

ddaspit left a comment

Enkidu93 left a comment

ddaspit left a comment

Enkidu93 commented Dec 4, 2023

ddaspit left a comment

Enkidu93 commented Dec 4, 2023

ddaspit left a comment

Enkidu93 left a comment

johnml1135 left a comment

ddaspit left a comment

Key terms support #229

Key terms support #229

Conversation

Enkidu93 commented Dec 1, 2023 • edited by ddaspit Loading

codecov-commenter commented Dec 1, 2023 • edited Loading

Codecov Report

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Dec 1, 2023

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Dec 1, 2023

johnml1135 commented Dec 4, 2023

Enkidu93 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 commented Dec 4, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

Enkidu93 commented Dec 4, 2023

Enkidu93 left a comment

Choose a reason for hiding this comment

johnml1135 commented Dec 4, 2023

johnml1135 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Dec 4, 2023

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Dec 4, 2023

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 left a comment

Choose a reason for hiding this comment

johnml1135 left a comment

Choose a reason for hiding this comment

ddaspit left a comment

Choose a reason for hiding this comment

Enkidu93 commented Dec 1, 2023 •

edited by ddaspit

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading