WIP: 103268-job-task-progress #104185

Nitin-Rawat-1 · 2023-02-17T13:13:34+01:00

Nitin-Rawat-1 commented

2023-02-17 13:13:34 +01:00

Issue #103268: This PR aims to implement the feature in the reference issue in the future. Right now approval of the project maintainer is required for the work done till now.

Nitin-Rawat-1 added 6 commits 2023-02-17 13:13:36 +01:00

OAPI: add endpoint to get task progress update from workers 7a1ae6928a

Add an endpoint for the Manager, to allow Workers, to send progress update of the task. Define the schema of the request body to hit this endpoint.

Issue: #103268

OAPI: add SocketIOTaskProgressUpdate for broadcasting progress updates 2053b25ecf

`SocketIOTaskProgressUpdate` objects are meant to be broadcast to SocketIO clients (i.e. the web interface). They are sent to the job specific room, just like task updates.

OAPI: regenerate code 168c786707

Worker: Extending blender render command bd86d0ff60

Blender render command is being extended in order to accept a frameRange property which is optional. FrameRange property will help in the calculation of the progress of a task.

Issue: 103268

Merge remote-tracking branch 'upstream/main' into 103268-job-task-progress 9d52a0556b

Merge remote-tracking branch 'upstream/main' into 103268-job-task-progress 09a3a4a58d

Nitin-Rawat-1 requested review from Sybren A. Stüvel 2023-02-17 14:12:52 +01:00

Nitin-Rawat-1 added 1 commit 2023-02-21 03:54:45 +01:00

Merge remote-tracking branch 'upstream/main' into 103268-job-task-progress 16cb71054f

Nitin-Rawat-1 added 1 commit 2023-02-21 10:45:40 +01:00

Merge remote-tracking branch 'upstream/main' into 103268-job-task-progress 43042e1845

Nitin-Rawat-1 added 1 commit 2023-02-23 10:38:03 +01:00

Merge remote-tracking branch 'upstream/main' into 103268-job-task-progress 430b490407

Nitin-Rawat-1 added 1 commit 2023-03-06 15:15:04 +01:00

Merge remote-tracking branch 'upstream/main' into 103268-job-task-progress 6ed046a87f

Nitin-Rawat-1 added 2 commits 2023-03-06 16:43:10 +01:00

OAPI: fix a typo 4ca61cc457

OAPI: regenerate code deef71e207

Nitin-Rawat-1 force-pushed 103268-job-task-progress from 2abfae2a0c to 218e476e78

2023-03-07 06:52:12 +01:00

Compare

Sybren A. Stüvel requested changes 2023-03-10 11:12:25 +01:00

Sybren A. Stüvel left a comment

I think this logic should be pulled into a separate function, and probably even use a different struct. That way the 'keeping track of frame processing progress' logic is separate from the 'rendering with Blender' code. I'm thinking along the lines of something like this:

package progress

// Indexed keeps track of the progress of indexed 'things'.
//
// This is typically used to keep track of the progress of rendering multiple
// frames in a single task.
type Indexed struct {
	completed map[int]bool // Map from index of the 'thing' to completion status.
	expected  int          // Number of things to complete before considering the work 'done'.
}

// NewIndexed returns a new indexed progress tracker for counting `expectedNumberOfThings` things.
func NewIndexed(expectedNumberOfThings int) Indexed {
	return Indexed{
		completed: make(map[int]bool),
		expected:  expectedNumberOfThings,
	}
}

// Completed marks the thing with the given index to be done.
// If this index was already marked 'done', this is a no-op.
func (isp *Indexed) Completed(index int) {
	isp.completed[index] = true
}

// IsDone returns true when at least the expected number of things have been marked 'completed'.
func (isp *Indexed) IsDone() bool {
	return isp.count() >= isp.expected
}

// Progress returns the progress so far, as (completed, total expected) counts.
func (isp *Indexed) Progress() (completed, totalExpected int) {
	completed = isp.count()
	totalExpected = isp.expected
	return
}

// count returns the number of indexed things that are marked 'completed'.
func (isp *Indexed) count() int {
	count := 0
	for _, value := range isp.completed {
		if value {
			count++
		}
	}
	return count
}

In theory a task can consist of multiple blender-render commands, though, so this will have to be tracked on the task level, not just the command level. It's probably a good idea to create some CommandProgressListener interface, and use that to bridge the command runner functions and the task executor.

I think this logic should be pulled into a separate function, and probably even use a different struct. That way the 'keeping track of frame processing progress' logic is separate from the 'rendering with Blender' code. I'm thinking along the lines of something like this: ```go package progress // Indexed keeps track of the progress of indexed 'things'. // // This is typically used to keep track of the progress of rendering multiple // frames in a single task. type Indexed struct { completed map[int]bool // Map from index of the 'thing' to completion status. expected int // Number of things to complete before considering the work 'done'. } // NewIndexed returns a new indexed progress tracker for counting `expectedNumberOfThings` things. func NewIndexed(expectedNumberOfThings int) Indexed { return Indexed{ completed: make(map[int]bool), expected: expectedNumberOfThings, } } // Completed marks the thing with the given index to be done. // If this index was already marked 'done', this is a no-op. func (isp *Indexed) Completed(index int) { isp.completed[index] = true } // IsDone returns true when at least the expected number of things have been marked 'completed'. func (isp *Indexed) IsDone() bool { return isp.count() >= isp.expected } // Progress returns the progress so far, as (completed, total expected) counts. func (isp *Indexed) Progress() (completed, totalExpected int) { completed = isp.count() totalExpected = isp.expected return } // count returns the number of indexed things that are marked 'completed'. func (isp *Indexed) count() int { count := 0 for _, value := range isp.completed { if value { count++ } } return count } ``` In theory a task can consist of multiple blender-render commands, though, so this will have to be tracked on the task level, not just the command level. It's probably a good idea to create some `CommandProgressListener` interface, and use that to bridge the command runner functions and the task executor.

internal/worker/command_blender.go

						
				@ -21,2 +22,4 @@

				var regexpFileSaved = regexp.MustCompile("Saved: '(.*)'")

				var regexpFrameNumber = regexp.MustCompile("Fra:[0-9]+")

				var prevFrame string

Sybren A. Stüvel commented

2023-03-10 10:41:58 +01:00

I don't see the need to declare these variables here.

internal/worker/command_blender.go

						
				@ -27,6 +32,7 @@ type BlenderParameters struct {

					argsBefore []string // Additional CLI arguments defined by the job compiler script, to go before the blend file name.

					blendfile  string   // Path of the file to open.

					args       []string // Additional CLI arguments defined by the job compiler script, to go after the blend file name.

					numFrames  float64  // Additional CLI argument defined by the job compiler script, to define the total number of frame that are needed to be rendered.

Sybren A. Stüvel commented

2023-03-10 10:40:10 +01:00

This is not a CLI argument. It's just for Flamenco's bookkeeping, and won't be passed to Blender in any way.

internal/worker/command_blender.go

						
				@ -47,2 +53,4 @@

					wg.Add(1)

					go func() {

						prevFrame = ""

						renderedNumFrames = 0

Sybren A. Stüvel commented

2023-03-10 10:41:08 +01:00

Why are these set here? They're not used in this goroutine. Also Go initializes variables to the zero value, so it's likely to be a no-op. It can cause a race condition, though, because the goroutine runs asynchronously from the rest of the code.

internal/worker/command_blender.go

						
				@ -140,1 +148,4 @@

					// Ignore the `ok` return value, as a missing numFrames key is fine:

					parameters.numFrames, _ = cmdParameter[float64](cmd, "numFrames")

					if parameters.numFrames != 0 {

Sybren A. Stüvel commented

2023-03-10 10:43:36 +01:00

This if is not necessary. totalNumFrames is initialized to zero anyway, so if paramters.numFrames == 0, the assignment can safely be done as it just reassigns zero.

This `if` is not necessary. `totalNumFrames` is initialized to zero anyway, so if `paramters.numFrames == 0`, the assignment can safely be done as it just reassigns zero.

Sybren A. Stüvel requested changes 2023-03-10 11:15:41 +01:00

internal/worker/command_exe.go Outdated

						
				@ -50,2 +52,3 @@

				//go:generate go run github.com/golang/mock/mockgen -destination mocks/cli_runner.gen.go -package mocks git.blender.org/flamenco/internal/worker CommandLineRunner

				// CommandLineRunner is an interface around exec.CommandContext().

				//

				//go:generate go run github.com/golang/mock/mockgen -destination mocks/cli_runner.gen.go -package mocks git.blender.org/flamenco/internal/worker CommandLineRunner

Sybren A. Stüvel commented

2023-03-10 11:13:29 +01:00

Try to keep the changes in a PR to a minimum. Unless there is a good reason, keep the ordering of these comments the same. That'll make it easier to see what actually changed.

internal/worker/listener.go

						
				@ -100,0 +109,4 @@

					if ctx.Err() != nil {

						return ctx.Err()

					}

					resp, err := l.client.TaskProgressUpdateWithResponse(ctx, taskID, progress)

Sybren A. Stüvel commented

2023-03-10 11:14:37 +01:00

This should not use the API client directly. If the Manager cannot be reached, the progress updates should be queued in the buffer, just like regular task updates.