Bad corpora should be flagged

[johnml1135]

If there is an extraneous tab in a corpora, it will crash the SMT build process with an error "Object must be of type String." This error is cryptic and does not allow the user to debug what is going on. It should either be clear "this file on this line is not correctly formatted - here is the correct format" or clean the files in a deterministic way before being passed down to the engines. This may need to be specific per engine type.

[Enkidu93]

Would you prefer a 'silently clean' solution or a 'throw an error from the machine side' solution? Seems like the latter might be more straightforward and less likely to cause difficult-to-debug headaches. But if it this trailing tab or similar issue is likely to be common, maybe it is just worth cleaning.

[johnml1135]

@Enkidu93, I would rather throw an intelligent error. If we clean, I would want to define it clearly - I would hate for people's data to be changed unexpectedly.

[Enkidu93]

OK, sounds good. I tried to recreate this by adding a tab with no luck (it completed and worked properly). Where did you add the extra tab?

[johnml1135]

At least before, this type of file caused errors:
MyFileWithOneTab.txt

my long sentence\t
my other long sentence
my third long sentence

Note the tab (\t) at the end of the first sentence
I believe the trailing tab specifically causes errors - in that it makes a zero length line as per these to pieces of code:
https://github.com/sillsdev/machine/blob/940ca147dc83638cc9c3da2128b7573af511381d/src/SIL.Machine/Corpora/TextFileAlignmentCollection.cs#L52-L59
https://github.com/sillsdev/machine/blob/940ca147dc83638cc9c3da2128b7573af511381d/src/SIL.Machine/Corpora/TextFileText.cs#L43-L48

The idea was seeing that but a bit more general. We have an odd format - either text per line or a keys (separated at times by '_' and by '-') that is then separated by a tab from the rest of the text.
A tab at the beginning, a tab at the end, keys separated by '-', a tab in a sentence that is unintentional - all of these would create bad data or crashing.
To remedy this here is a proposal:

• When uploading files, if they are of type "text", run a "text" check:
  • Verify that all lines either have one tab or none
• Change the behavior of separating keys to only underscores "_". Document it in the API. Update the code in TextFileAlignmentCollection.cs to only split on underscores.

[ddaspit]

What is the issue with dashes in a key?

[ddaspit]

As @Enkidu93 said, we could make the code a bit more fault-tolerant and simply ignore any trailing whitespace, such as tabs, when reading the text file.

[johnml1135]

(1) the issue with the dashes is that it is inconsistent (used in one place but not the other) and undocumented. trimming white space sounds fine - that is unless we get a whole raft of verse references with no sentences! I would be more inclined to have the clients clean the data and then they can establish the intention better. It is really a data format that we are specifying - I wouldn't want to "clean" csv files saying "you didn't follow the rules, but I think this is what you intended". I would rather say "here are the rules, and if you don't follow them, I will error out and tell you why."

[ddaspit]

We can document that we support either dashes or underscores to separate different components of a key.

I would like to keep the Machine corpus classes as fault-tolerant as possible, since those are often used by researchers, but I don't have a problem with being more strict in Serval and validating the files when they are uploaded.

[Enkidu93]

So besides updating the documentation in machine, all edits should be in serval, right? I.e., we're making it serval's responsibility to validate file format? Also, are the file formats clearly documented somewhere? (I've figured out what they are myself from fixing the echo engine, but I didn't know if there were an official description somewhere).

[johnml1135]

@Enkidu93 - Let's have Serval document and check the file format. The only documentation is what is in this issue - and what we can determine from the code. I am still inclined to only use one separator (and even switch it to a semi-colon (;) for added clarity). I don't want verses (i.e., Mat 1:3-7) to trigger the separator.

[Enkidu93]

As I understand it, a text file can have one of two formats:

1. Lines of text (no tabs - only spaces and newlines at the end of each line).
2. Lines of text with one or two tabs in one of the following formats: a key, a tab, and then text; a key and a tab (empty); a key, a tab, text, a tab, a column code (ss, ir, rr...).

So I should be able to validate with a regex pretty easily. @johnml1135, could you verify that the above is correct? Thank you!

[Enkidu93]

Sorry, didn't see your reply til I posted.

[Enkidu93]

@johnml1135 I'm going off of what I learned with Peter Chapman about formats for the echo engine. I don't think verses like those should be a problem - maybe the column codes account for the formatting issues? Semicolons are also present in many verses though - depending on the version - and they're also more English-specific since punctuation is very variable across languages. Seems like a special character like tab might be better, right? I'm up for anything though.

[johnml1135]

What about bar |? It’s visually very clear and I don’t think it would be used in verse references.

[Enkidu93]

That might work. I know that some languages use a danda which looks like that for punctuation but that's a different unicode character I believe. I do wonder if that could be confusing though or difficult given that other keyboards may not have it?? Why the dislike of the tab? Also, is this assumption about tabs already deeply ingrained in any projects using these tools?

[johnml1135]

The tab idea is growing on me. Since it is a non-visual character, it will not conflict with any other script. any visual punctuation carries the risk of being used in some scripts versus. Then, each reference would be separated by a tab, and after the last tab would be the actual segment. There should be no tabs at the beginning or end of the line.

[ddaspit]

I would prefer that you not make significant changes to the text file format for backwards compatibility purposes in the Machine libraries. It is currently working as-is with SF, so there is no need to change it. All we need to do is validate the current format in Serval.

[johnml1135]

If that’s the case, let’s just use the underscore to separate references and document it. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Damien Daspit ***@***.***> Sent: Thursday, August 10, 2023 5:12:33 PM To: sillsdev/serval ***@***.***> Cc: John Lambert ***@***.***>; Mention ***@***.***> Subject: Re: [sillsdev/serval] Bad corpora should be flagged (Issue #2) I would prefer that you not make significant changes to the text file format for backwards compatibility purposes in the Machine libraries. It is currently working as-is with SF, so there is no need to change it. All we need to do is validate the current format in Serval. — Reply to this email directly, view it on GitHub<#2 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADIY5NHPAIFVUKHHJQNEFG3XUVFEDANCNFSM6AAAAAAXG5Y7OQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[johnml1135]

One edge case to make sure we cover is where there is a reference, but no segment text (and/or there is text and then an ending tab). My best understanding is we should just ignore the segment completely - it does not add anything to us.
The other edge case I can imagine is where the source and target references don't align (because they are matched by line number, not reference). A few ways to handle this:

• Ignore any target references, and document that this is what is happening (my preferred path)
• Concatenate the two sets of references (too complex - how is it benefiting the client?)

[Enkidu93]

I think a regex like this should cover it, right? (\S := any non-whitespace charater)

"^(((\S| )*\n)+|((\S+\t(\S| )*(\t\S+)?)\n))+$"

or in more readable notation:

((\S + \space)(\S + \space)* + \S\S*\t(\S + \space)(\S + \space)*(\t\S\S* + \empty))\n

[Enkidu93]

One edge case to make sure we cover is where there is a reference, but no segment text (and/or there is text and then an ending tab). My best understanding is we should just ignore the segment completely - it does not add anything to us. The other edge case I can imagine is where the source and target references don't align (because they are matched by line number, not reference). A few ways to handle this:

* Ignore any target references, and document that this is what is happening (my preferred path)

* Concatenate the two sets of references (too complex - how is it benefiting the client?)

So the first issue is if the source line is empty? And the second issue is if there are more target lines than source lines or target lines that have no source reference?

[ddaspit]

@johnml1135 If I am understanding your edge cases correctly, Machine should handle both of these cases gracefully. Empty segments and segments that do not have a matching pair are simply ignored.

[johnml1135]

If I am correct then, we can add:

documentation for text files:

• All text files have one source or target segment per line with no tabs in the segment
• Text files can optionally have references associated with each line
• If references are used, a few things must be followed:
  • Each line must contain a tab to separate references and the segment, even if there are no references or text in the segment
  • Multiple references can be added, separated by an underscore (_), such as: Ref 1_Ref-2-with-dashes_1 JON 2:18-21
  • All starting and ending whitespace will be trimmed from references and segments
  • For parallel text, segments are associated by line number AND all references for every line must match between the source and target side, or else the line/segment will be ignored

Checking and feeback:

• When creating a corpus, enhance the response to include:
  • For each parallel text (matching textId) return the following information:
  • Number of total lines in the files
  • Number of parallel segments successfully read in
  • Number of source only segments successfully read in
  • Whether there are references in the text or not
  • StatusMessage - either "All lines successfully read in and associated" or, output a message as appropriate based upon the mal-formed inputs.
  • The StatusMessage can be concatenated and placed in the MongoDB to be recalled when requesting info about the corpora.

@ddaspit Is this all accurate? Would anything else need to be said?

[Enkidu93]

@johnml1135 I'm confused about the multiple references element. So in the example data I've used, there are references like 40_1 or chapter_01_verse_001. My impression was that these each constituted a single reference, not a set {40, 1} or {chapter, 01, verse, 001}. When the references are used in the textId selection for pretranslation, they are referred to as a unit, right? i.e. 40_1. Is that just parsed as two refs that happen to only correspond to "40_1"? Wouldn't that imply a M:N relationship between source segments and target segments? I'm just a little confused. Where is this logic? Is this documented anywhere?

Otherwise, I'm on board with what you said, and I think my regex should be sufficient for validation (given the tests I've run on sample data). And I think the logic on the machine side already handles the missing refs issue.

[ddaspit]

@johnml1135 As @Enkidu93 said, the underscore/dash does not indicate multiple references. It indicates multiple components of a single reference. It is what I refer to as a MultiKeyRef. The different components/keys are used for sorting purposes. For example, if you wanted to represent a verse ref that is sorted correctly, you would construct JOHN 1:1 like this 40JHN_01_001. The rest seems accurate.

[Enkidu93]

Hey, folks! I'm running out of MVP issues I can make progress on alone, so I'm moving back to this. I have basic validation working and updated the docs (see https://github.com/sillsdev/serval/tree/handle_malformatted_files_%232). As for the additional edits @johnml1135 suggested (i.e., checking and feedback), I'm not sure how to go about incorporating those since - as I understand it - aligning the source and target lines is done machine-side. I understand the utility of including this information in the AddCorpus and GetCorpus replies, but I'm not sure how to cleanly include it without doing the same work twice (which seems like a bad move in many ways esp. maintaining consistency) or creating a more complex system of calls between Serval and Machine (and complexity ∝brokenness).

[Enkidu93]

@johnml1135 Thoughts?

[johnml1135]

@Enkidu93 and @ddaspit - how about doing the checking when actually building the engine? It would be in one place and either (1) it would fail with an error message about a specific corpora or (2) it would return "statistics" about the build process as a new field in the Mongo database (time to build, number of matching verses, number pretranslated - as a text output).

[ddaspit]

I think it makes sense to let the engines validate the data. It is already the responsibility of the engine to load/parse the data. This does mean that different engines might validate the data differently. One engine might be more forgiving than another.

The engine model already has a corpusSize property that is filled in by the engine during building. We could add some additional stats if that is useful.

[ddaspit]

I would say that at this point, we should probably only add new features if SF has a specific requirement for it.

[ddaspit]

The Machine engine is already essentially validating the data when it parses it during a build and will return an error if it is unable to do so. We could add a strict parsing mode to the TextFileTextCorpus class by adding a flag to the constructor.

[Enkidu93]

I think it makes sense to let the engines validate the data. It is already the responsibility of the engine to load/parse the data. This does mean that different engines might validate the data differently. One engine might be more forgiving than another.

The engine model already has a corpusSize property that is filled in by the engine during building. We could add some additional stats if that is useful.

That makes sense to me too. The only complaint I would have is that, as a user, I now have to wait until building to see if the text files I uploaded are valid, so it may still make sense to inform the user if individual text files are invalid (not corpus-level issues, just file-level). I can imagine a situation where I upload 100s of files, and they're all malformed. I go through the process only to find at the build stage that there's an issue. But maybe this is better dealt with at the SF-level - I don't know.

I would say that at this point, we should probably only add new features if SF has a specific requirement for it.

So do you think we should not pursue this? What do you mean by this?

The Machine engine is already essentially validating the data when it parses it during a build and will return an error if it is unable to do so. We could add a strict parsing mode to the TextFileTextCorpus class by adding a flag to the constructor.

Sure, and the initial issue-posting acknowledges that, but isn't the point to make these errors more descriptive/fixable?

[ddaspit]

I think it makes sense to let the engines validate the data. It is already the responsibility of the engine to load/parse the data. This does mean that different engines might validate the data differently. One engine might be more forgiving than another.

The engine model already has a corpusSize property that is filled in by the engine during building. We could add some additional stats if that is useful.

That makes sense to me too. The only complaint I would have is that, as a user, I now have to wait until building to see if the text files I uploaded are valid, so it may still make sense to inform the user if individual text files are invalid (not corpus-level issues, just file-level). I can imagine a situation where I upload 100s of files, and they're all malformed. I go through the process only to find at the build stage that there's an issue. But maybe this is better dealt with at the SF-level - I don't know.

Since Serval is a web service for applications and not users, I think we can safely push the responsibility for more user-friendly handling of malformed data to the application. Returning an error at build-time is probably sufficient at this point. Validating the data requires a background job of some type. We can't block a request while we validate a potentially large corpus. This would require a complex design/API that supports asynchronous processing and notification when it is finished validating. We don't know how big of an issue this will be for calling applications, so I would prefer to wait to implement something this complex until we have a specific requirement for it.

I would say that at this point, we should probably only add new features if SF has a specific requirement for it.

So do you think we should not pursue this? What do you mean by this?

I think that adding additional statistics isn't necessary at this point. SF doesn't have a specific requirement for it, so we can defer that to a future release.

The Machine engine is already essentially validating the data when it parses it during a build and will return an error if it is unable to do so. We could add a strict parsing mode to the TextFileTextCorpus class by adding a flag to the constructor.

Sure, and the initial issue-posting acknowledges that, but isn't the point to make these errors more descriptive/fixable?

I was saying that we could make the errors more descriptive and add more strict validation in the TextFileTextCorpus class. This would deal with the issue without have to make any changes to Serval or the Machine engine service.

[Enkidu93]

I was saying that we could make the errors more descriptive and add more strict validation in the TextFileTextCorpus class. This would deal with the issue without have to make any changes to Serval or the Machine engine service.

OK, I see. I'm realizing though: Is there really any corpus-level validation necessary? I suppose the only thing we might check is if a corpus is all target content and no source content. Thoughts? What ought to be checked here? If it's the case that we don't need any corpus-level validation, then we can just move the validation I had worked on in Serval to Machine on a per-text basis and, like you said, hide it behind a constructor flag. (And if we need more specific error-reporting (i.e., "unexpected token in " rather than just " is not formatted correctly"), we can do something iterative rather than just a regex validation). How does that sound?

[johnml1135]

Validation may be just throwing a descriptive error if we were unable to process a file. It would be nice if we could identify the specific line that was failing and why. Beyond that, I don't think we need more validation.

[Enkidu93]

OK, I'll go ahead then.