studio/flamenco
studio
/
flamenco
12
27
Fork 32
Code Issues 60 Pull Requests 7 Releases Wiki Activity

Worker: check BLENDER_CMD environment variable (for multi-GPU Eevee rendering) #104193

Open
MKRelax wants to merge 9 commits from MKRelax/flamenco:worker-use-blender-from-env into main
pull from: MKRelax/flamenco:worker-use-blender-from-env
merge into: studio:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 12 Commits 9 Files Changed 11 +394 -208
MKRelax commented 2023-03-01 18:27:49 +01:00
Contributor
Copy Link

Worker update: if "blender" is specified by the manager, check the BLENDER_CMD environment variable for a Blender path (before checking $PATH and .blend file association).

Use case: Assigning a specific GPU to each worker, e.g. for Eevee rendering on multiple GPUs at the same time.

Usage:

  • Copy blender.exe to e.g. blender-gpu0.exe and blender-gpu1.exe;
  • Assign a different GPU to each, through the OS or NVidia Control Panel (3D Settings, Program Settings. Specify OpenGL Rendering GPU for Eevee and/or CUDA Rendering GPU for Cycles);
  • Create a startup script for each worker, providing it not only a unique FLAMECO_HOME and FLAMENCO_WORKERNAME, but also BLENDER_CMD with a full path to the desired blender-gpu?.exe
  • Each worker will then use its own blender-gpu?.exe and in turn, use its own GPU for rendering.

Known issues / improvements:

  • On startup, the worker will still report the Blender version it finds on the path or via file association. To correct this, find_blender.CheckBlender() should be modified, but that code is (too) tightly coupled with meta.FindBlenderExePath(), which is also used by the manager.
  • On Linux+NVidia, one could set __NV_PRIME_RENDER_OFFLOAD for similar behaviour.

Test cases are provided. Please review the code because this is my first Go code ever ;)

Worker update: if "blender" is specified by the manager, check the `BLENDER_CMD` environment variable for a Blender path (before checking `$PATH` and `.blend` file association). Use case: Assigning a specific GPU to each worker, e.g. for Eevee rendering on multiple GPUs at the same time. Usage: * Copy `blender.exe` to e.g. `blender-gpu0.exe` and `blender-gpu1.exe`; * Assign a different GPU to each, through the OS or NVidia Control Panel (3D Settings, Program Settings. Specify `OpenGL Rendering GPU` for Eevee and/or `CUDA Rendering GPU` for Cycles); * Create a startup script for each worker, providing it not only a unique `FLAMECO_HOME` and `FLAMENCO_WORKERNAME`, but also `BLENDER_CMD` with a full path to the desired `blender-gpu?.exe` * Each worker will then use its own `blender-gpu?.exe` and in turn, use its own GPU for rendering. Known issues / improvements: * On startup, the worker will still report the Blender version it finds on the path or via file association. To correct this, `find_blender.CheckBlender()` should be modified, but that code is (too) tightly coupled with `meta.FindBlenderExePath()`, which is also used by the manager. * On Linux+NVidia, one could set `__NV_PRIME_RENDER_OFFLOAD` for similar behaviour. Test cases are provided. Please review the code because this is my first Go code ever ;)
MKRelax added 1 commit 2023-03-01 18:27:50 +01:00
Sybren A. Stüvel requested changes 2023-03-03 12:51:41 +01:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Thanks for the PR! This looks useful indeed.

I think the environment var should be named FLAMENCO_BLENDER_PATH. This would make it consistent with the other FLAMENCO_... variables, and makes it clear that it should contain a path. For me a 'command' would be something that could also include CLI arguments to run.

And extra 👍 for writing unit tests :)

On startup, the worker will still report the Blender version it finds on the path or via file association.

This is an issue, indeed. If you want to refactor/redesign meta.FindBlenderExePath() or how it is coupled with find_blender.CheckBlender() that would be great.

On Linux+NVidia, one could set __NV_PRIME_RENDER_OFFLOAD for similar behaviour.

Good to know :)

Thanks for the PR! This looks useful indeed. I think the environment var should be named `FLAMENCO_BLENDER_PATH`. This would make it consistent with the other `FLAMENCO_...` variables, and makes it clear that it should contain a path. For me a 'command' would be something that could also include CLI arguments to run. And extra :+1: for writing unit tests :) > On startup, the worker will still report the Blender version it finds on the path or via file association. This is an issue, indeed. If you want to refactor/redesign `meta.FindBlenderExePath()` or how it is coupled with `find_blender.CheckBlender()` that would be great. > On Linux+NVidia, one could set __NV_PRIME_RENDER_OFFLOAD for similar behaviour. Good to know :)
internal/find_blender/find_blender.go Outdated
@ -52,1 +52,4 @@
// EnvironmentVariable returns the content of the BLENDER_CMD environment variable.
func EnvironmentVariable() string {
return os.Getenv("BLENDER_CMD")
Sybren A. Stüvel commented 2023-03-03 12:45:03 +01:00
Owner
Copy Link

I think the name of this variable should be in a constant. Can be in the same file. That way the string literal doesn't get repeated everywhere.

I think the name of this variable should be in a constant. Can be in the same file. That way the string literal doesn't get repeated everywhere.
MKRelax marked this conversation as resolved
internal/find_blender/find_blender_test.go Outdated
@ -44,0 +46,4 @@
func TestGetBlenderCommandFromEnvironment(t *testing.T) {
// Without environment variable, we expect an empty string
path := EnvironmentVariable()
Sybren A. Stüvel commented 2023-03-03 12:45:38 +01:00
Owner
Copy Link

This should check that the environment variable isn't already set while running the test ;-)

This should check that the environment variable isn't already set while running the test ;-)
MKRelax marked this conversation as resolved
internal/find_blender/find_blender_test.go Outdated
@ -44,0 +52,4 @@
// Try finding the blender path in the BLENDER_CMD environment variable
err := os.Setenv("BLENDER_CMD", "/path/specified/in/env/to/blender")
if err != nil {
t.Fatal("Could not set BLENDER_CMD environment variable")
Sybren A. Stüvel commented 2023-03-03 12:46:18 +01:00
Owner
Copy Link

The if and t.Fatal() can be replaced with require.NoError(t, err, "Could not set BLENDER_CMD environment variable").

The `if` and `t.Fatal()` can be replaced with `require.NoError(t, err, "Could not set BLENDER_CMD environment variable")`.
MKRelax marked this conversation as resolved
internal/worker/command_blender.go Outdated
@ -93,0 +90,4 @@
// No directory path given. Check that the executable can be found in the
// environment variable BLENDER_CMD or on the path.
if path := find_blender.EnvironmentVariable(); path != "" {
logger.Info().Str("path", path).Msg("found Blender in environment")
Sybren A. Stüvel commented 2023-03-03 12:43:40 +01:00
Owner
Copy Link

I wouldn't say "found Blender" here, because the path wasn't checked. Blender may not even exist there.

Maybe find_blender.EnvironmentVariable() can return two strings, (name, path) where name is the name of the environment variable, and path is the actual value. That way you can log here "using Blender from {environment variable name}".

I wouldn't say "found Blender" here, because the path wasn't checked. Blender may not even exist there. Maybe `find_blender.EnvironmentVariable()` can return two strings, `(name, path)` where `name` is the name of the environment variable, and `path` is the actual value. That way you can log here "using Blender from {environment variable name}".
MKRelax commented 2023-03-04 02:59:10 +01:00
Author
Contributor
Copy Link

I did change the log message to "using Blender from FLAMENCO_BLENDER_PATH".

However, since the name of the variable is (now) a constant I think there is no need to return it from find_blender.EnvironmentVariable(). Most code would not be interested in it anyway and probably ignore it using a _ variable. So the log message simply uses the constant.

I did change the log message to "using Blender from FLAMENCO_BLENDER_PATH". However, since the name of the variable is (now) a constant I think there is no need to return it from `find_blender.EnvironmentVariable()`. Most code would not be interested in it anyway and probably ignore it using a `_` variable. So the log message simply uses the constant.
Sybren A. Stüvel commented 2023-03-06 10:50:45 +01:00
Owner
Copy Link

I think it's still good to return the variable name. That way you get less coupling between the different parts of the code, making things more maintainable.

I think it's still good to return the variable name. That way you get less coupling between the different parts of the code, making things more maintainable.
MKRelax added 4 commits 2023-03-04 02:37:23 +01:00
MKRelax force-pushed worker-use-blender-from-env from da2ed4e69b to a2d9612dc2 2023-03-04 03:40:34 +01:00 Compare
MKRelax force-pushed worker-use-blender-from-env from a2d9612dc2 to 070ef06921 2023-03-04 04:54:14 +01:00 Compare
MKRelax force-pushed worker-use-blender-from-env from 070ef06921 to 3416a5ff54 2023-03-04 05:02:54 +01:00 Compare
MKRelax commented 2023-03-04 06:17:13 +01:00
Author
Contributor
Copy Link

All of your suggestions have been implemented including renaming to FLAMENCO_BLENDER_PATH, see commits.

I agree that the worker MUST report the correct Blender version that it would use if the manager does not specify a path. That has been implemented by adding a FLAMENCO_BLENDER_PATH check to find_blender.CheckBlender(). However, this code is also used by the manager and there are a few considerations:

  • In find_blender.CheckBlender(), the full_path check (if crosspath.Dir(exename) != ".") was moved above the file association check (if exename==""). This works even when exename is empty, because crosspath.Dir("") returns "." in that case and the execution flow continues as before.
    The $FLAMENCO_BLENDER_PATH check has been inserted between the two, so the order is 1) full_path, 2) $FLAMENCO_BLENDER_PATH, 3) .blend association, 4) $PATH. This also leaves the CheckBlenderExePath API call unaffected, which uses: 1) full_path.

The $FLAMENCO_BLENDER_PATH environment variable would normally not be present on the manager. But IF it is, that could impact the managers Setup Assistant:

  • No .blend file association or $PATH checks would be performed because the $FLAMENCO_BLENDER_PATH takes precedence;
  • The returned path would be ignored by the manager because it has the type api.BlenderPathSourceInputPath (input_path).
    We could add a new type to the api.BlenderPathSource (e.g. blender_path_env), but such a "Blender Path Source" has no meaning on the manager and does not belong in the API imo. Also the path_envvar type did not apply since a) the executable may not be on the $PATH and s) when selected, the manager would store "" in the config file;
  • Result (if the env var is present): The manager would only show the input field for "Another Blender executable" in this (rare) case.

Improving this would require a refactoring of the blender-path / blender-version stuff find_blender.CheckBlender(), meta.FindBlenderExePath() and meta.CheckBlenderExePath(). I'm not sure if I'm up to that, because I have no Go experience whatsoever. But I could try if you want me to!

For now, this pull request should implement the FLAMENCO_BLENDER_PATH functionality on the worker, including correct logging output, with minimal impact on the manager.

Please review again, feedback is welcome! Also, I could squash commits if that would make things easier, just let me know.

All of your suggestions have been implemented including renaming to `FLAMENCO_BLENDER_PATH`, see commits. I agree that the worker MUST report the correct Blender version that it would use if the manager does not specify a path. That has been implemented by adding a `FLAMENCO_BLENDER_PATH` check to `find_blender.CheckBlender()`. However, this code is also used by the manager and there are a few considerations: - In `find_blender.CheckBlender()`, the full_path check (`if crosspath.Dir(exename) != "."`) was moved above the file association check (`if exename==""`). This works even when `exename` is empty, because `crosspath.Dir("")` returns `"."` in that case and the execution flow continues as before. The `$FLAMENCO_BLENDER_PATH` check has been inserted between the two, so the order is 1) full_path, 2) `$FLAMENCO_BLENDER_PATH`, 3) `.blend` association, 4) `$PATH`. This also leaves the `CheckBlenderExePath` API call unaffected, which uses: 1) full_path. The `$FLAMENCO_BLENDER_PATH` environment variable would normally not be present on the manager. But IF it is, that could impact the managers Setup Assistant: - No .blend file association or $PATH checks would be performed because the $FLAMENCO_BLENDER_PATH takes precedence; - The returned path would be ignored by the manager because it has the type `api.BlenderPathSourceInputPath` (`input_path`). We could add a new type to the `api.BlenderPathSource` (e.g. `blender_path_env`), but such a "Blender Path Source" has no meaning on the manager and does not belong in the API imo. Also the `path_envvar` type did not apply since a) the executable may not be on the $PATH and s) when selected, the manager would store "" in the config file; - Result (if the env var is present): The manager would only show the input field for "Another Blender executable" in this (rare) case. Improving this would require a refactoring of the blender-path / blender-version stuff `find_blender.CheckBlender()`, `meta.FindBlenderExePath()` and `meta.CheckBlenderExePath()`. I'm not sure if I'm up to that, because I have no Go experience whatsoever. But I could try if you want me to! For now, this pull request should implement the `FLAMENCO_BLENDER_PATH` functionality on the worker, including correct logging output, with minimal impact on the manager. Please review again, feedback is welcome! Also, I could squash commits if that would make things easier, just let me know.
MKRelax changed title from WIP: Worker: check BLENDER_CMD environment variable (for multi-GPU Eevee rendering) to Worker: check BLENDER_CMD environment variable (for multi-GPU Eevee rendering) 2023-03-04 06:19:24 +01:00
MKRelax requested review from Sybren A. Stüvel 2023-03-04 06:19:31 +01:00
Sybren A. Stüvel commented 2023-03-06 12:11:08 +01:00
Owner
Copy Link

Please review again, feedback is welcome! Also, I could squash commits if that would make things easier, just let me know.

I'll do another review this week, thanks for the patch and the clear description of your reasonings. That'll make things a lot easier :)

Squashing the commits isn't necessary, in the end it's going to be squashed anyway.

> Please review again, feedback is welcome! Also, I could squash commits if that would make things easier, just let me know. I'll do another review this week, thanks for the patch and the clear description of your reasonings. That'll make things a lot easier :) Squashing the commits isn't necessary, in the end it's going to be squashed anyway.
Sybren A. Stüvel requested changes 2023-03-13 16:05:19 +01:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Things are improving, but the API changes on the find_blender module could be better.

There is still a strong coupling between find_blender.EnvironmentVariable() and the code in internal/worker/command_blender.go. Moving things into a constant hasn't changed this, as the code in command_blender.go still needs to know that EnvironmentVariable() is using that particular constant.

Most code would not be interested in it anyway and probably ignore it using a _ variable.

This is a rather weak argument, as currently literally 100% of the production (i.e. non-test) code that uses this function is interested in it.

Looking at the code a bit more, I think it would be a good idea to push things a little further, and make EnvironmentVariable() return a tuple (CheckBlenderResult, error), just like the other functions. The api.BlenderPathSource enum-like type can be expanded to include this new environment variable, and then it's officially part of the API.

Things are improving, but the API changes on the `find_blender` module could be better. There is still a strong coupling between `find_blender.EnvironmentVariable()` and the code in `internal/worker/command_blender.go`. Moving things into a constant hasn't changed this, as the code in `command_blender.go` still needs to know that `EnvironmentVariable()` is using that particular constant. > Most code would not be interested in it anyway and probably ignore it using a `_` variable. This is a rather weak argument, as currently literally 100% of the production (i.e. non-test) code that uses this function is interested in it. Looking at the code a bit more, I think it would be a good idea to push things a little further, and make `EnvironmentVariable()` return a tuple `(CheckBlenderResult, error)`, just like the other functions. The `api.BlenderPathSource` enum-like type can be expanded to include this new environment variable, and then it's officially part of the API.
MKRelax commented 2023-03-13 17:43:56 +01:00
Author
Contributor
Copy Link

Ok, I fully agree.

Adding this to the API would mean that the Manager would need to handle this kind of path whenever it receives it. That would be the case if $FLAMENCO_BLENDER_PATH happens to be set in the managers' environment. Again I don't see a use case for that, but IF that variable is set, I guess we would need to present that path and its source/reason to the user just like with 'file association' and 'path'.

There's one small thing that bothers me about that. IF the variable is set, a user would probably target worker(s) on that machine to use it. If the manager picks up on it, put that path in flamenco-manager.yaml and pass it to workers, that would effectively prevent workers from using $FLAMENCO_BLENDER_PATH themselves, because they only to that if they get 'blender' from the manager (which basically means "use your own Blender").
I'm not sure, but I believe if the manager finds Blender through its own $PATH, it passes 'blender' (not the path itself) to workers to instruct them to use (their) $PATH as well. I think we should do the same if the manager finds blender through the $FLAMENCO_BLENDER_PATH: also pass 'blender' to workers to allow them to use their own $FLAMENCO_BLENDER_PATH or $PATH.

Regarding the BlenderPathSource enum, would you prefer blender_path_envvar (which is short, but might be too similar to the existing path_envvar) or flamenco_blender_path_envvar (which is long, but consistent because path_envvar corresponds to $PATH and flamenco_blender_path_envvar corresponds to $FLAMENCO_BLENDER_PATH)?

Ok, I fully agree. Adding this to the API would mean that the Manager would need to handle this kind of path whenever it receives it. That would be the case if `$FLAMENCO_BLENDER_PATH` happens to be set in the managers' environment. Again I don't see a use case for that, but IF that variable is set, I guess we would need to present that path and its source/reason to the user just like with 'file association' and 'path'. There's one small thing that bothers me about that. IF the variable is set, a user would probably target worker(s) on that machine to use it. If the manager picks up on it, put that path in `flamenco-manager.yaml` and pass it to workers, that would effectively *prevent* workers from using `$FLAMENCO_BLENDER_PATH` themselves, because they only to that if they get 'blender' from the manager (which basically means "use your own Blender"). I'm not sure, but I believe if the manager finds Blender through its own `$PATH`, it passes 'blender' (not the path itself) to workers to instruct them to use (their) `$PATH` as well. I think we should do the same if the manager finds blender through the `$FLAMENCO_BLENDER_PATH`: also pass 'blender' to workers to allow them to use their own `$FLAMENCO_BLENDER_PATH` or `$PATH`. Regarding the `BlenderPathSource` enum, would you prefer `blender_path_envvar` (which is short, but might be too similar to the existing `path_envvar`) or `flamenco_blender_path_envvar` (which is long, but consistent because `path_envvar` corresponds to `$PATH` and `flamenco_blender_path_envvar` corresponds to `$FLAMENCO_BLENDER_PATH`)?
Sybren A. Stüvel commented 2023-03-14 13:43:51 +01:00
Owner
Copy Link

Ok, I fully agree.

👍

Adding this to the API would mean that the Manager would need to handle this kind of path whenever it receives it. That would be the case if $FLAMENCO_BLENDER_PATH happens to be set in the managers' environment. Again I don't see a use case for that, but IF that variable is set, I guess we would need to present that path and its source/reason to the user just like with 'file association' and 'path'.

I agree that this would be better to split up somehow, and only have the Workers look at $FLAMENCO_BLENDER_PATH. It would be a bigger change, but I agree that this would be cleaner.

Having said that, that (unification of Manager and Worker behaviour) wasn't my intention with 'adding it to the API'. My intention was to turn this:

func EnvironmentVariable() string {
	return os.Getenv(BlenderPathEnvVariable)
}

into something like this (code untested, just to illustrate my thoughts):

func EnvironmentVariable(ctx context.Context) (CheckBlenderResult, error) {
	exePath := os.Getenv(BlenderPathEnvVariable)
	if exePath == "" {
		return CheckBlenderResult{}, ErrEnvVariableNotSet
	}
	return getResultWithVersion(
		ctx,
		BlenderPathEnvVariable,
		exePath,
		api.BlenderPathSourceEnvVariable)
}

For this to work, the BlenderPathSource enum in pkg/api/flamenco-openapi.yaml would have to be expanded with env_variable.

There's one small thing that bothers me about that. IF the variable is set, a user would probably target worker(s) on that machine to use it. If the manager picks up on it, put that path in flamenco-manager.yaml and pass it to workers, that would effectively prevent workers from using $FLAMENCO_BLENDER_PATH themselves, because they only to that if they get 'blender' from the manager (which basically means "use your own Blender").

Yeah, that's a good thing to prevent. How I see it (but feel free to come up with counter-arguments): if the Manager passes a path, the Worker should use that. If the Manager just says blender, the Worker is free to use $FLAMENCO_BLENDER_PATH, $PATH, .blend file associations, etc.

I'm not sure, but I believe if the manager finds Blender through its own $PATH, it passes 'blender' (not the path itself) to workers to instruct them to use (their) $PATH as well. I think we should do the same if the manager finds blender through the $FLAMENCO_BLENDER_PATH: also pass 'blender' to workers to allow them to use their own $FLAMENCO_BLENDER_PATH or $PATH.

With the above, I think this is no longer relevant, as I think the Manager simply shouldn't use that environment variable.

This does touch on #100195 though. Maybe it's something you would find interesting working on at some point?

Regarding the BlenderPathSource enum, would you prefer blender_path_envvar (which is short, but might be too similar to the existing path_envvar) or flamenco_blender_path_envvar (which is long, but consistent because path_envvar corresponds to $PATH and flamenco_blender_path_envvar corresponds to $FLAMENCO_BLENDER_PATH)?

I think env_variable is good enough. I don't think we'll be adding even more environment variables for this, so just having an option that indicates "that one variable that we use for this stuff" is IMO good enough.

> Ok, I fully agree. :+1: > Adding this to the API would mean that the Manager would need to handle this kind of path whenever it receives it. That would be the case if `$FLAMENCO_BLENDER_PATH` happens to be set in the managers' environment. Again I don't see a use case for that, but IF that variable is set, I guess we would need to present that path and its source/reason to the user just like with 'file association' and 'path'. I agree that this would be better to split up somehow, and only have the Workers look at `$FLAMENCO_BLENDER_PATH`. It would be a bigger change, but I agree that this would be cleaner. Having said that, that (unification of Manager and Worker behaviour) wasn't my intention with 'adding it to the API'. My intention was to turn this: ```go func EnvironmentVariable() string { return os.Getenv(BlenderPathEnvVariable) } ``` into something like this (code untested, just to illustrate my thoughts): ```go func EnvironmentVariable(ctx context.Context) (CheckBlenderResult, error) { exePath := os.Getenv(BlenderPathEnvVariable) if exePath == "" { return CheckBlenderResult{}, ErrEnvVariableNotSet } return getResultWithVersion( ctx, BlenderPathEnvVariable, exePath, api.BlenderPathSourceEnvVariable) } ``` For this to work, the `BlenderPathSource` enum in `pkg/api/flamenco-openapi.yaml` would have to be expanded with `env_variable`. > There's one small thing that bothers me about that. IF the variable is set, a user would probably target worker(s) on that machine to use it. If the manager picks up on it, put that path in `flamenco-manager.yaml` and pass it to workers, that would effectively *prevent* workers from using `$FLAMENCO_BLENDER_PATH` themselves, because they only to that if they get 'blender' from the manager (which basically means "use your own Blender"). Yeah, that's a good thing to prevent. How I see it (but feel free to come up with counter-arguments): if the Manager passes a path, the Worker should use that. If the Manager just says `blender`, the Worker is free to use `$FLAMENCO_BLENDER_PATH`, `$PATH`, `.blend` file associations, etc. > I'm not sure, but I believe if the manager finds Blender through its own `$PATH`, it passes 'blender' (not the path itself) to workers to instruct them to use (their) `$PATH` as well. I think we should do the same if the manager finds blender through the `$FLAMENCO_BLENDER_PATH`: also pass 'blender' to workers to allow them to use their own `$FLAMENCO_BLENDER_PATH` or `$PATH`. With the above, I think this is no longer relevant, as I think the Manager simply shouldn't use that environment variable. This does touch on #100195 though. Maybe it's something you would find interesting working on at some point? > Regarding the `BlenderPathSource` enum, would you prefer `blender_path_envvar` (which is short, but might be too similar to the existing `path_envvar`) or `flamenco_blender_path_envvar` (which is long, but consistent because `path_envvar` corresponds to `$PATH` and `flamenco_blender_path_envvar` corresponds to `$FLAMENCO_BLENDER_PATH`)? I think `env_variable` is good enough. I don't think we'll be adding even more environment variables for this, so just having an option that indicates "that one variable that we use for this stuff" is IMO good enough.
MKRelax commented 2023-03-14 16:52:46 +01:00
Author
Contributor
Copy Link

Yeah, that's a good thing to prevent. How I see it (but feel free to come up with counter-arguments): if the Manager passes a path, the Worker should use that. If the Manager just says blender, the Worker is free to use $FLAMENCO_BLENDER_PATH, $PATH, .blend file associations, etc.

We've briefly discussed this this week, then you explained that the design is to have "Workers as dumb as possible, with as little configuration as possible". I agree with that, however the backside of this is that the Manager is trying to be "too smart" while it actually is not. A few thoughts:

  • When starting the Manager, it tries to find a path to Blender, but it does not actually need or use Blender itself. I know its just trying to be helpful, but its not really that helpful:
  • The manager's Blender path has little meaning. It's at best, a reasonable suggestion for workers, in particular for workers on the same machine but always limited to workers that happen to use the same OS.
  • If provided with a specific path (say "C:\Bin\Blender 3.4\blender.exe" on Windows), the manager currently copies that path in flamenco-manager.yaml not only for the windows platform (which again, might or might not be a reasonable suggestion for workers) but also for the linux and darwin platforms (which is definitely wrong). Similarly on Linux, it would use the given Linux path (say "/usr/local/bin/blender") for the windows and darwin platforms (which is definitely wrong for Windows workers). Fact is, a manager simply has no clue about reasonable paths for other platforms, so it should not try to be smart about it. The best it could do is pass "blender" to other platforms (which would be better than passing Windows paths to Linux workers and vice versa).
  • When given full paths (not "blender"), the manager doesn't suggest it to workers of that OS, but is rather forceful about it. This might keep workers 'dumb', but it forces system administrators (who have to install Blender anyway, and the workers) to have Blender installed in that particular directory.
  • Not all workers are managed by the same system administrator. In fact, workers could serve different managers on demand (offering their services to one manager today, another manager tomorrow). Managers passing hard-coded paths would not be viable in those cases, they would almost always fall back to the "blender" implementation instead.
  • If the manager does not find blender, apparently that is a problem (#100195). This prevents people from running a Manager on a NAS (which would make sense; having the manager and the rendered files storage on a NAS).
  • All this "path" functionality seems to be specifically targeted at blender. From a "managing jobs" perspective, I don't see how running blender is any different from running ffmpeg, echo, sleep or some other executable like gaffer (in the chat recently). At the moment echo and sleep use the OS commands and ffmpeg is bundled, but for any other executables like gaffer, workers would probably use their $PATH. Questions are: if the managers tell workers which blender path they should use, would it also force which gaffer path to use? Or the other way: if we wouldn't pass hardcoded gaffer paths, why do we do that for blender?
  • Finally: having a manager decide which executable is to be used by a worker seems like a security vulnerability to me. A malicious manager could probably get a worker to execute "/usr/bin/rm" instead of blender...

My suggestions would be:

  • the "blender" implementation (or more generic "": not supplying a path) should be the default for all executables on all platforms, i.e. workers decide for themselves which executables they use, using a) flamenco-worker.yaml, b) environment variable like FLAMENCO_{cmd}_PATH e.g. FLAMENCO_BLENDER_PATH or FLAMENCO_GAFFER_PATH, c) bundled files like ffmpeg,d) file association if available, e) $PATH.
  • Workers would in most cases use bundled software or their $PATH, which keeps them "dumb" enough IMO.
  • For system administrators, specifying paths in flamenco-worker.yaml would be quite logical and straightforward. Using environment variables would be more 'advanced', but still quite common practice in bat/sh/bash scripts. Both would keep system administrators in control of which executables are run by workers.
  • I personally don't see a use case for a manager demanding workers which path they must use (and I think its dangerous), but given the current implementation there must be one. So: the wizard COULD allow users to provide paths (for all executables, and for all supported platforms) in case they would like to override the default behaviour (or to keep close to the existing behaviour).
  • For the managers' own OS, e.g. when started on a Windows machine, the manager COULD come up with some reasonable suggestions, based on file associations or local $PATH. But that's quite far-fetched IMO.

Feel free to move this discussion somewhere else, but the essence of my PR is that in some cases, workers want to decide for themselves which executables to use (in this case: to force which GPU to use, but that's just one of many reasons). So relevant to this PR.

In the mean time, I will work on the func EnvironmentVariable() API. I agree with that, thanks for the feedback.

> Yeah, that's a good thing to prevent. How I see it (but feel free to come up with counter-arguments): if the Manager passes a path, the Worker should use that. If the Manager just says blender, the Worker is free to use $FLAMENCO_BLENDER_PATH, $PATH, .blend file associations, etc. We've briefly discussed this this week, then you explained that the design is to have "Workers as dumb as possible, with as little configuration as possible". I agree with that, however the backside of this is that the Manager is trying to be "too smart" while it actually is not. A few thoughts: - When starting the Manager, it tries to find a path to Blender, but it does not actually *need* or *use* Blender itself. I know its just trying to be helpful, but its not really that helpful: - The manager's Blender path has little meaning. It's at best, a reasonable *suggestion* for workers, *in particular* for workers on the same machine but always *limited to* workers that happen to use the same OS. - If provided with a specific path (say `"C:\Bin\Blender 3.4\blender.exe"` on Windows), the manager currently copies that path in `flamenco-manager.yaml` not only for the `windows` platform (which again, *might or might not be* a reasonable suggestion for workers) but *also* for the `linux` and `darwin` platforms (which is definitely wrong). Similarly on Linux, it would use the given Linux path (say `"/usr/local/bin/blender"`) for the `windows` and `darwin` platforms (which is definitely wrong for Windows workers). Fact is, a manager simply has no clue about reasonable paths for other platforms, so it should not try to be smart about it. The best it could do is pass "blender" to other platforms (which would be better than passing Windows paths to Linux workers and vice versa). - When given full paths (not "blender"), the manager doesn't *suggest* it to workers of that OS, but is rather forceful about it. This might keep workers 'dumb', but it forces system administrators (who have to install Blender anyway, and the workers) to have Blender installed in that particular directory. - Not all workers are managed by the same system administrator. In fact, workers could serve different managers on demand (offering their services to one manager today, another manager tomorrow). Managers passing hard-coded paths would not be viable in those cases, they would almost always fall back to the "blender" implementation instead. - If the manager does not find blender, apparently that is a problem (#100195). This prevents people from running a Manager on a NAS (which would make sense; having the manager and the rendered files storage on a NAS). - All this "path" functionality seems to be specifically targeted at blender. From a "managing jobs" perspective, I don't see how running `blender` is any different from running `ffmpeg`, `echo`, `sleep` or some other executable like `gaffer` (in the chat recently). At the moment `echo` and `sleep` use the OS commands and `ffmpeg` is bundled, but for any other executables like `gaffer`, workers would probably use their $PATH. Questions are: if the managers tell workers which `blender` path they should use, would it also force which `gaffer` path to use? Or the other way: if we wouldn't pass hardcoded `gaffer` paths, why do we do that for `blender`? - Finally: having a manager decide which executable is to be used by a worker seems like a security vulnerability to me. A malicious manager could probably get a worker to execute "/usr/bin/rm" instead of blender... My suggestions would be: - the `"blender"` implementation (or more generic `""`: not supplying a path) should be the default for all executables on all platforms, i.e. workers decide for themselves which executables they use, using a) `flamenco-worker.yaml`, b) environment variable like `FLAMENCO_{cmd}_PATH` e.g. `FLAMENCO_BLENDER_PATH` or `FLAMENCO_GAFFER_PATH`, c) bundled files like `ffmpeg`,d) file association if available, e) `$PATH`. - Workers would in most cases use bundled software or their `$PATH`, which keeps them "dumb" enough IMO. - For system administrators, specifying paths in `flamenco-worker.yaml` would be quite logical and straightforward. Using environment variables would be more 'advanced', but still quite common practice in bat/sh/bash scripts. Both would keep system administrators in control of which executables are run by workers. - I personally don't see a use case for a manager demanding workers which path they must use (and I think its dangerous), but given the current implementation there must be one. So: the wizard COULD allow users to provide paths (for all executables, and for all supported platforms) in case they would like to override the default behaviour (or to keep close to the existing behaviour). - For the managers' own OS, e.g. when started on a Windows machine, the manager COULD come up with some reasonable suggestions, based on file associations or local $PATH. But that's quite far-fetched IMO. Feel free to move this discussion somewhere else, but the essence of my PR is that in some cases, workers want to decide for themselves which executables to use (in this case: to force which GPU to use, but that's just one of many reasons). So relevant to this PR. In the mean time, I will work on the `func EnvironmentVariable()` API. I agree with that, thanks for the feedback.
Sybren A. Stüvel commented 2023-03-14 17:58:41 +01:00
Owner
Copy Link
  • When starting the Manager, it tries to find a path to Blender, but it does not actually need or use Blender itself. I know its just trying to be helpful, but its not really that helpful:

Let's not discuss the topic of #100195 right here. IMO those are two separate things.

  • The manager's Blender path has little meaning. It's at best, a reasonable suggestion for workers, in particular for workers on the same machine but always limited to workers that happen to use the same OS.

This is exactly the setup we have at Blender Animation Studio, where all machines use Blender from the same path. It's quite simple to work with.

  • When given full paths (not "blender"), the manager doesn't suggest it to workers of that OS, but is rather forceful about it. This might keep workers 'dumb', but it forces system administrators (who have to install Blender anyway, and the workers) to have Blender installed in that particular directory.

Yes, indeed. Instead of configuring each and every worker individually, Flamenco assumes that you have configured them identically and keeps as much as the configuration in a single location.

  • Not all workers are managed by the same system administrator.

This is something that is out of scope for Flamenco itself. Flamenco is made for individual and smaller studios. If bigger studios can/want to use it, that's fantastic and certainly welcome. It's not something that the design was aiming at, though.

You can read more about this in https://studio.blender.org/blog/flamenco-3-a-new-beginning/, I'll have to put that into the Flamenco documentation as well at some point.

In fact, workers could serve different managers on demand (offering their services to one manager today, another manager tomorrow).

This is entirely out of scope for the project. Again, if you want to use Flamenco like this and it works for you, that's amazing, but it's not part of Flamenco's design.

  • Finally: having a manager decide which executable is to be used by a worker seems like a security vulnerability to me. A malicious manager could probably get a worker to execute "/usr/bin/rm" instead of blender...

Just like you can run Blender with blender --python-expr 'import os; os.removedirs('/') and it'll have the same effect and work in a cross-platform way as well.

Flamenco was made with specific goals in mind: rendering films with Blender. 90% of it was implemented in just 7 months by a single person. Of course there are going to be limitations to what it can do.

> - When starting the Manager, it tries to find a path to Blender, but it does not actually *need* or *use* Blender itself. I know its just trying to be helpful, but its not really that helpful: Let's not discuss the topic of #100195 right here. IMO those are two separate things. > - The manager's Blender path has little meaning. It's at best, a reasonable *suggestion* for workers, *in particular* for workers on the same machine but always *limited to* workers that happen to use the same OS. This is exactly the setup we have at Blender Animation Studio, where all machines use Blender from the same path. It's quite simple to work with. > - When given full paths (not "blender"), the manager doesn't *suggest* it to workers of that OS, but is rather forceful about it. This might keep workers 'dumb', but it forces system administrators (who have to install Blender anyway, and the workers) to have Blender installed in that particular directory. Yes, indeed. Instead of configuring each and every worker individually, Flamenco assumes that you have configured them identically and keeps as much as the configuration in a single location. > - Not all workers are managed by the same system administrator. This is something that is out of scope for Flamenco itself. Flamenco is made for individual and smaller studios. If bigger studios can/want to use it, that's fantastic and certainly welcome. It's not something that the design was aiming at, though. You can read more about this in https://studio.blender.org/blog/flamenco-3-a-new-beginning/, I'll have to put that into the Flamenco documentation as well at some point. > In fact, workers could serve different managers on demand (offering their services to one manager today, another manager tomorrow). This is entirely out of scope for the project. Again, if you want to use Flamenco like this and it works for you, that's amazing, but it's not part of Flamenco's design. > - Finally: having a manager decide which executable is to be used by a worker seems like a security vulnerability to me. A malicious manager could probably get a worker to execute "/usr/bin/rm" instead of blender... Just like you can run Blender with `blender --python-expr 'import os; os.removedirs('/')` and it'll have the same effect and work in a cross-platform way as well. Flamenco was made with specific goals in mind: rendering films with Blender. 90% of it was implemented in just 7 months by a single person. Of course there are going to be limitations to what it can do.
MKRelax commented 2023-03-14 18:33:15 +01:00
Author
Contributor
Copy Link

This is in no way meant to be a rant, and I apologize if it came across like that.

With multi-GPU Eevee rendering as goal, I am (lacking better options) looking for a way to have a worker use a specific executable. This contradicts with the paradigm "if the Manager passes a path, the Worker should use that" and you asked me to "feel free to come up with counter-arguments". That's all I intended to do. I very much respect Flamenco, the redesign of v3, the effort you put in it and you as a developer.

For this PR, I will adjust the func EnvironmentVariable() as per your comments and add env_variable to BlenderPathSource enumeration. I the (rare) case the a Manager receives such a env_variable path, it would behave the same as with file association or $PATH paths and put "blender" in the configuration file.

This is in no way meant to be a rant, and I apologize if it came across like that. With multi-GPU Eevee rendering as goal, I am (lacking better options) looking for a way to have a worker use a specific executable. This contradicts with the paradigm "if the Manager passes a path, the Worker should use that" and you asked me to "feel free to come up with counter-arguments". That's all I intended to do. I very much respect Flamenco, the redesign of v3, the effort you put in it and you as a developer. For this PR, I will adjust the `func EnvironmentVariable()` as per your comments and add `env_variable` to `BlenderPathSource` enumeration. I the (rare) case the a Manager receives such a `env_variable` path, it would behave the same as with `file association` or `$PATH` paths and put `"blender"` in the configuration file.
Sybren A. Stüvel commented 2023-03-14 18:41:17 +01:00
Owner
Copy Link

This is in no way meant to be a rant, and I apologize if it came across like that.

🙏

With multi-GPU Eevee rendering as goal, I am (lacking better options) looking for a way to have a worker use a specific executable. This contradicts with the paradigm "if the Manager passes a path, the Worker should use that"

My thought was that with a setup like yours, you could use "blender" as the configured path in the Manager, in which case the environment variable would be used on the Worker. Wouldn't that suit your needs? Or would it be more convenient to have that envvar override whatever the Manager is giving?

Now that I think of it some more, since envvars can't change while the Worker is running, it can log the location of Blender it would use. That would make things more visible, working around the issue that environment variables are (for most users) invisible things.

and you asked me to "feel free to come up with counter-arguments". That's all I intended to do. I very much respect Flamenco, the redesign of v3, the effort you put in it and you as a developer.

Thanks.

For this PR, I will adjust the func EnvironmentVariable() as per your comments and add env_variable to BlenderPathSource enumeration. I the (rare) case the a Manager receives such a env_variable path, it would behave the same as with file association or $PATH paths and put "blender" in the configuration file.

That's fine; if it turns out to be a practical issue, we can look at solutions then.

> This is in no way meant to be a rant, and I apologize if it came across like that. 🙏 > With multi-GPU Eevee rendering as goal, I am (lacking better options) looking for a way to have a worker use a specific executable. This contradicts with the paradigm "if the Manager passes a path, the Worker should use that" My thought was that with a setup like yours, you could use `"blender"` as the configured path in the Manager, in which case the environment variable would be used on the Worker. Wouldn't that suit your needs? Or would it be more convenient to have that envvar override whatever the Manager is giving? Now that I think of it some more, since envvars can't change while the Worker is running, it can log the location of Blender it would use. That would make things more visible, working around the issue that environment variables are (for most users) invisible things. > and you asked me to "feel free to come up with counter-arguments". That's all I intended to do. I very much respect Flamenco, the redesign of v3, the effort you put in it and you as a developer. Thanks. > For this PR, I will adjust the `func EnvironmentVariable()` as per your comments and add `env_variable` to `BlenderPathSource` enumeration. I the (rare) case the a Manager receives such a `env_variable` path, it would behave the same as with `file association` or `$PATH` paths and put `"blender"` in the configuration file. That's fine; if it turns out to be a practical issue, we can look at solutions then.
viktorasm reviewed 2023-12-15 15:59:12 +01:00
Sybren A. Stüvel commented 2024-02-24 12:02:09 +01:00
Owner
Copy Link

Hi @MKRelax , do you still have an interest in working on this?

Hi @MKRelax , do you still have an interest in working on this?
MKRelax commented 2024-02-24 18:51:27 +01:00
Author
Contributor
Copy Link

@dr.sybren It's been a while indeed...

From a functional point of view, I think it's very valueable to have workers use their own instance of the Blender executable. In my case, I have 8 workers on 5 machines each running exclusively on their own GPU.
On Linux+NVidia, one could set __NV_PRIME_RENDER_OFFLOAD environment variable to configure which GPU to use for which process. On Windows workers, one needs separate executables e.g. blender-gpu0.exe and blender-gpu1.exe, each configured to use a specific GPU.
To keep the knowledge about the exact .exe names on the worker, and to keep things similar to Linux scripts, using environment variables to configure workers seems a viable solution.

If I remember correctly, there were a few hurdles to overcome:

  1. The "find blender" code is shared between manager and worker. This may be problematic, for example if a manager would find blender-gpu0.exe in its environment and instruct all workers to use that. I'd rather have a) the manager not look at FLAMENCO_BLENDER_PATH at all or b) the workers being able to overrule the managers suggestion for the blender path (which seems a bit against the 'dumb worker' philosophy).
  2. We discussed that the EnvironmentVariable() function should return a getResultWithVersion. However, to do that, we would need to add env_variable to the BlenderPathSource options of the OpenAPI. That would make "setting an environment variable" a full-fledged way to configure the blender path, also on the manager. Similar to 1), we would need to decide on what that would actually mean, what purpose it serves and how workers should deal with it if they have environment settings of their own.
  3. Based on @victorasm comment: the CheckBlender function might need some refactoring since it already calls itself recursively and also checking for environment variables makes things "too messed up".

From a technical/practical point of view, I'm not a Go developer and I'm usually on Windows which makes compiling Flamenco a bit of a hassle (had to modify the makefile). For example. I have no idea how to regenerate the API codebase after adding to BlenderPathSource. I might switch over to MinGW-w64, WSL or an Ubuntu machine to make things easier, but it is a reason I left this aside for some time. Also I don't really need to, because the proposed changes have been working beautifully for me for many months now. I mainly just wanted to share the idea and the code. I would like to make it a better fit to the codebase but I lack both Go experience and insights on Flamenco's architecture.

From an architecture point of view, you made clear that Flamenco was developed from a studio perspective (where a manager actively manages "dumb" workers) while I personally have a "internet-of-workers" perspective (where all my machines (Windows desktops/laptops/game PC's, Linux servers, mining rigs) become workers that serve one manager today (e.g. from my home office) and another manager tomorrow (e.g. from some client office). Given your (now deleted) remark about me "ranting" about things that are "out of scope", there is a bit of a mismatch there. But let me state again that it's great that Flamenco serves both use cases.
Given that the technical changes are relatively small and the multi-GPU setup is discussed in the FAQ of this project (and some people reached out and copied my setup), it might be worthwhile to persue this, but also more efficient to have someone work on this with more Go/Flamenco experience.

@dr.sybren It's been a while indeed... From a functional point of view, I think it's very valueable to have workers use their own instance of the Blender executable. In my case, I have 8 workers on 5 machines each running exclusively on their own GPU. On Linux+NVidia, one could set `__NV_PRIME_RENDER_OFFLOAD` environment variable to configure which GPU to use for which process. On Windows workers, one needs separate executables e.g. `blender-gpu0.exe` and `blender-gpu1.exe`, each configured to use a specific GPU. To keep the knowledge about the exact `.exe` names on the worker, and to keep things similar to Linux scripts, using environment variables to configure workers seems a viable solution. If I remember correctly, there were a few hurdles to overcome: 1) The "find blender" code is shared between manager and worker. This may be problematic, for example if a manager would find `blender-gpu0.exe` in its environment and instruct all workers to use that. I'd rather have a) the manager not look at `FLAMENCO_BLENDER_PATH` at all or b) the workers being able to overrule the managers suggestion for the blender path (which seems a bit against the 'dumb worker' philosophy). 2) We discussed that the `EnvironmentVariable()` function should return a `getResultWithVersion`. However, to do that, we would need to add `env_variable` to the `BlenderPathSource` options of the OpenAPI. That would make "setting an environment variable" a full-fledged way to configure the blender path, also on the manager. Similar to 1), we would need to decide on what that would actually mean, what purpose it serves and how workers should deal with it if they have environment settings of their own. 3) Based on @victorasm comment: the `CheckBlender` function might need some refactoring since it already calls itself recursively and also checking for environment variables makes things "too messed up". From a technical/practical point of view, I'm not a Go developer and I'm usually on Windows which makes compiling Flamenco a bit of a hassle (had to modify the makefile). For example. I have no idea how to regenerate the API codebase after adding to `BlenderPathSource`. I might switch over to MinGW-w64, WSL or an Ubuntu machine to make things easier, but it is a reason I left this aside for some time. Also I don't really need to, because the proposed changes have been working beautifully for me for many months now. I mainly just wanted to share the idea and the code. I would like to make it a better fit to the codebase but I lack both Go experience and insights on Flamenco's architecture. From an architecture point of view, you made clear that Flamenco was developed from a studio perspective (where a manager actively manages "dumb" workers) while I personally have a "internet-of-workers" perspective (where all my machines (Windows desktops/laptops/game PC's, Linux servers, mining rigs) become workers that serve one manager today (e.g. from my home office) and another manager tomorrow (e.g. from some client office). Given your (now deleted) remark about me "ranting" about things that are "out of scope", there is a bit of a mismatch there. But let me state again that it's great that Flamenco serves both use cases. Given that the technical changes are relatively small and the multi-GPU setup is discussed in the FAQ of this project (and some people reached out and copied my setup), it might be worthwhile to persue this, but also more efficient to have someone work on this with more Go/Flamenco experience.
MKRelax force-pushed worker-use-blender-from-env from 3416a5ff54 to 9d77497e51 2024-02-25 01:09:13 +01:00 Compare
MKRelax added 3 commits 2024-02-25 05:29:49 +01:00
MKRelax added 1 commit 2024-02-25 06:09:28 +01:00
MKRelax commented 2024-02-25 06:19:34 +01:00
Author
Contributor
Copy Link

I moved my files to WSL (Linux) which makes development much easier. Updated the OAPI and now returning a api.BlenderPathSourceEnvVariable for environment paths. The EnvironmentVariable function also returns the name of the variable and and err if the file does not exist or is a directory. In the testcases, I pass the go file itself as Blender executable because it must exist, a bit of a hack.
Since configuring by env_variable is officially part of the API now, it is also being picked up by the manager so for completeness, I have added this option to the manager Setup Assistant. It has no usecase IMO but if we leave it in, it should probably be documented.
The FindBlender code has not been refactored so as @victorasm commented before, it is probably still "too messed up".
I will test this PR in practice the next couple of days.

I moved my files to WSL (Linux) which makes development much easier. Updated the OAPI and now returning a `api.BlenderPathSourceEnvVariable` for environment paths. The `EnvironmentVariable` function also returns the name of the variable and and `err` if the file does not exist or is a directory. In the testcases, I pass the go file itself as Blender executable because it must exist, a bit of a hack. Since configuring by `env_variable` is officially part of the API now, it is also being picked up by the manager so for completeness, I have added this option to the manager Setup Assistant. It has no usecase IMO but if we leave it in, it should probably be documented. The `FindBlender` code has not been refactored so as @victorasm commented before, it is probably still "too messed up". I will test this PR in practice the next couple of days.
MKRelax force-pushed worker-use-blender-from-env from 42660941e5 to 46ab0075fb 2024-02-25 22:19:22 +01:00 Compare
Sybren A. Stüvel commented 2024-02-26 09:27:53 +01:00
Owner
Copy Link

The "find blender" code is shared between manager and worker

That's easy to separate. Also there's #100195 for that.

The other two points I agree with, it's better to clean some things up before expanding the functionality.

And yeah, building on Windows is a bit sub-par, that's why I put "Convert the build toolchain to Mage to simplify development and packaging on Windows." on the Google Summer of Code ideas list. Hopefully somebody can help with that.

I moved my files to WSL (Linux) which makes development much easier. Updated the OAPI and now returning a api.BlenderPathSourceEnvVariable for environment paths. The EnvironmentVariable function also returns the name of the variable and and err if the file does not exist or is a directory.

That sounds good!

In the testcases, I pass the go file itself as Blender executable because it must exist, a bit of a hack.

Which go file? (I haven't had time to actually look at the code changes yet)

Since configuring by env_variable is officially part of the API now, it is also being picked up by the manager so for completeness, I have added this option to the manager Setup Assistant. It has no usecase IMO but if we leave it in, it should probably be documented.

👍

The FindBlender code has not been refactored so as @victorasm commented before, it is probably still "too messed up".

Ok. As long as it's covered with unit tests, then refactoring later should be fine.

I will test this PR in practice the next couple of days.

👍 Just press the "Re-request Review" button if you feel the patch is ready for reviewing. It's in the reviewers list next to my name.

> The "find blender" code is shared between manager and worker That's easy to separate. Also there's #100195 for that. The other two points I agree with, it's better to clean some things up before expanding the functionality. And yeah, building on Windows is a bit sub-par, that's why I put "Convert the build toolchain to Mage to simplify development and packaging on Windows." on the [Google Summer of Code ideas list](https://developer.blender.org/docs/programs/gsoc/ideas/#flamenco). Hopefully somebody can help with that. > I moved my files to WSL (Linux) which makes development much easier. Updated the OAPI and now returning a `api.BlenderPathSourceEnvVariable` for environment paths. The `EnvironmentVariable` function also returns the name of the variable and and `err` if the file does not exist or is a directory. That sounds good! > In the testcases, I pass the go file itself as Blender executable because it must exist, a bit of a hack. Which go file? (I haven't had time to actually look at the code changes yet) > Since configuring by `env_variable` is officially part of the API now, it is also being picked up by the manager so for completeness, I have added this option to the manager Setup Assistant. It has no usecase IMO but if we leave it in, it should probably be documented. :+1: > The `FindBlender` code has not been refactored so as @victorasm commented before, it is probably still "too messed up". Ok. As long as it's covered with unit tests, then refactoring later should be fine. > I will test this PR in practice the next couple of days. :+1: Just press the "Re-request Review" button if you feel the patch is ready for reviewing. It's in the reviewers list next to my name.
This pull request has changes conflicting with the target branch.
  • internal/worker/command_blender_test.go
  • pkg/api/openapi_spec.gen.go
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u worker-use-blender-from-env:MKRelax-worker-use-blender-from-env
git checkout MKRelax-worker-use-blender-from-env

Merge

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff MKRelax-worker-use-blender-from-env
git checkout main
git merge --ff-only MKRelax-worker-use-blender-from-env
git checkout MKRelax-worker-use-blender-from-env
git rebase main
git checkout main
git merge --no-ff MKRelax-worker-use-blender-from-env
git checkout main
git merge --squash MKRelax-worker-use-blender-from-env
git checkout main
git merge MKRelax-worker-use-blender-from-env
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
Good First Issue
  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Status
Archived

  
Status
Confirmed

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Good First Issue
Priority
High
Priority
Low
Priority
Normal
Status
Archived
Status
Confirmed
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: studio/flamenco#104193
No description provided.
Delete Branch "MKRelax/flamenco:worker-use-blender-from-env"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?