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

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 97 at r1 (raw file):

            buildOptionsObject = JsonSerializer.Deserialize<JsonObject>(buildOptions);

        using MemoryStream sourceStream = new();

What exactly is prompting this change?

This is only needed for train and not for pretranslate?

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

This brings it back to what it was before. For some reason, the Json writer works async.

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @Enkidu93)

OK

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

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

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 116 at r3 (raw file):

                if (row.SourceSegment.Length > 0 || row.TargetSegment.Length > 0)
                {
                    await sourceTrainWriter.WriteLineAsync(row.SourceSegment);

WriteLineAsync uses the OS-specific newline characters. I would prefer that we revert to the previous behavior where it always uses \n, so that the behavior is consistent across platforms.

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

                if (row.SourceSegment.Length > 0 && row.TargetSegment.Length == 0)
                {
                    pretranslateWriter.WriteStartObject();

I spent a bit of time trying to understand how to properly use Utf8JsonWriter in an async context. This class is intended for low-level, optimized use cases. It does not automatically flush the buffer to the underlying stream. It only writes when the class is disposed or FlushAsync is explicitly called. In order to avoid writing all pretranslations to memory before flushing, we should periodically call FlushAsync. We can use the BytesPending property to flush after "x" amount of data is in the buffer.

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

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

We could flush every time 4096 (or something like that) bytes are pending.

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

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

I'm confused. The S3WriteStream is specifically designed to upload in chunks. If we are receiving an exception from writing to the stream multiple times, then it sounds like a bug. @Enkidu93 Do you have any idea what is going on here?

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit)

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

You mentioned getting an AmazonS3Exception: What line is throwing that? Or could you point me to a failed E2E?

OK, seems like you have a plan - what precisely do you think is going wrong that having two buffers would fix?

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @johnml1135)

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

Can you give more details about what is happening? I don't understand what "have the whole file ready" means. It doesn't sound like double buffering is the right solution.

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @johnml1135)

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

It is more likely that we are simply calling the S3 API incorrectly.

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @johnml1135)

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

I think I understand what is going on. S3 has a minimum part size of 5MB, unless it is the last part of the file, so we need to buffer the stream until we have 5MB of data to send a part. Is that correct?

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

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 126 at r3 (raw file):

Good work figuring that out. This change will make the S3 code more robust.

src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 138 at r6 (raw file):

                }
                if (pretranslateWriter.BytesPending > 1024 * 1024)
                    pretranslateWriter.FlushAsync();

You should await the FlushAsync call.

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