Manager: allow setup to finish without Blender #104306
No reviewers
Sybren A. Stüvel
Labels
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
Job Type
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: studio/flamenco#104306
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "abelli/flamenco:issue100195"
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?
Overview
This patch updates the step where the Manager cannot find blender with an option to skip.
If skipped, the end configuration file will have the blender variable set to just
blender.Fix #100195
Demo
Generated
flamenco-manager.yamlThanks for the PR, it's fixing a long-standing eye-sore :)
I feel that the code structure could use a bit of work. There's now two ways in which the 'use whatever Blender' setting is represented:
skipCustomBlenderExebooleanthis.selectedBlenderas set bycheckBlenderExePath().I think it would be better to have another possible value of
selectedBlender.source, and have that set via the "use whatever Blender" radio button. That way there's less juggling of different representations of the same user choice.This would require altering the OpenAPI definition in
flamenco-manager.yaml, to extend theBlenderPathSourceschema. For that I'd recommend reading Generating Code and OpenAPI commit guidelines@ -103,2 +103,3 @@@back-clicked="prevStep":is-next-clickable="selectedBlender != null || customBlenderExe != (null || '')":is-next-clickable="selectedBlender != null || customBlenderExe != (null || '') || skipCustomBlenderExeThat original code already won't work,
customBlenderExe != (null || '')is not going to do what it's intended to express.nullis a false-y value, and so(null || '')gets shortcut to just'').That's not something to address in this PR though -- this PR adds a feature, and shouldn't also try and fix existing code. But if you're willing to look into this too, a separate PR that fixes this (which could then be landed before adding this new feature) would be much appreciated.
I have opened #104307 to address this one.
Manager: allow setup to finish without Blenderto WIP: Manager: allow setup to finish without BlenderWIP: Manager: allow setup to finish without Blenderto Manager: allow setup to finish without BlenderHello @dr.sybren I'm able to generate and extend the OpenAPI definition but I do have a questions on how to proceed.
I'd like to confirm if by using a bit more of work could mean some more drastic changes to the original code, like refactoring and doing some arrangements to turn the "use whatever Blender" logic into one.
So that it not only can produce a
selectedBlenderobject with the source attribute "input_path" but also "skip_input".With the switch being our radio buttons.
👍
Yes. As an overall perspective on coding: this is Open Source, which means it's yours. It's kind of a min-max game:
In other words, avoid the "this is only my little feature, I'll just tack it onto the existing code" pitfall that so many of us fall into (including me).
Yep. But for the API I wouldn't use the term
skip_input-- that name reflects the logic for the UI, but it does not reflect the data itself. I thinkdefaultwould be better, as it should cause Flamenco to simply use the default"blender"string. By skipping this choice (UI), the user gets this default value (API).I did some updates to the code, it didn't felt like drastic changes were required as I previously thought, but I tried to follow your tips for a better coding result.
Although the updated code looks similar my latest commit will not work. I'd like to ask your help to solve this, so far here is what happens:
selectedBlenderwith the return object ofblenderFromDefaultSource()A POST request is made to http://192.168.0.16:8080/api/v3/configuration/setup-assistant
with the following payload:
And my final
flamenco-manager.yamlwill have the blender variables empty.If I updated the source attribute to be
input_pathinstead ofdefaultit will generate aflamenco-manager.yamlwith my blender variables as just "blender", as expected.I think this payload has a few too many fields set. With
source="default"it shouldn't be necessary to provide any other value. It's a matter of 'ownership': The Manager should be in control over what those defaults are, not the web interface.Sure, but that's trying to work around an issue in the Manager API by changing the webapp. That's not the right way to go.
I think you've uncovered two issues at once:
400 Bad Requestresponse in such a case.source="default", which should be added.I had to keep the payload because if I only required the
sourcefield inBlenderPathCheckResultof flamenco-openapi.yaml It would cause errors on code that depended on the other fields, for exampleBlenderExecutable.IsUsablein meta.goI don't like it and I wish to do it properly. I'd like to request your help again 😅
What errors?
When I run
make generateit gives me the errors below.I assume it's because of fields that were previously required are now unable to be used as before.
That's to be expected. You'll have to make the existing code work with the now-optional fields. Basically the code handling any other case than
source="default"needs to check that the pointers are notniland return errors when they are.Hi, I updated the code and decided to keep the payload to just
{ source: 'default', isUsable: true }because of the many checks present in both frontend and API implementation layer that will not allow us to proceed withoutisUsablebeing a truthy value.When I was fixing the
meta_test.gofile I created an anonymous struct to serve as a mocked payload so that I could feed the newly converted pointer fields. I'm not sure if this is the proper way of doing, so please let me know.I also encountered an edge case on a if statement in
SaveSetupAssistantConfigfrommeta.go. The problem was that, at least inGoit will stop evaluating our conditions as soon as it gets satisfied first, so a long conditional chain won't go deep if the first link yields something.To solve it, I created two boolean variables
isConfigInvalid, responsible to check if we have core fields andisConfigIncompleteto check if optional fields were not supplied in the payload.So the operation is the following, check first if we have a valid config, using Go's principle mentioned above, evaluate and fail immediately if we don't, OR (assuming it is valid) if source is not 'default' and we don't have the optional fields, fail as well.
I believe that this should also cover the bug where it accepts values that lead to an invalid state, but please let me know what do you think and if it makes sense.
👍
This is not the right way to go. Go doesn't allow you to take the address of a literal, but you can use a little trick to make it almost work that way: have a function return a pointer to its argument. In some of the test files you'll already see this little function:
which makes it possible to say
somePointer = ptr("some literal"). This function should only be used in tests, though, as it's a bit of a hacky one. For non-test code, I think this would be better:This is common, it's in pretty much every programming language I know of. It's called 'short-circuiting'.
The code now got a bit hairy, as it tries to mix two quite different cases into one.
default: theinputandpathfields should not be given.Mixing those cases into one makes it harder to write the conditions, so it's better to split them out.
Another complication is that, now that so many of the fields are optional, there's two different "no value" states:
nilor"", and both should be handled as "no information". It's not enough to just test fornil.So with those complications, do you still want to keep those fields optional? Or make them required and have explicit empty strings there? I think it'll make a lot of the code easier.
You could also investigate keeping them optional and giving them an empty string as default value in the
flamenco-openapi.yaml, but I think that won't be reflected in the generated Go code (it'll likely still be pointers).👍 That's interesting, thanks for the trick!
I've tried splitting them as suggested so I'd have a bool variable to check if source is
defaultand if we do (not) haveinputandpath.Then, I had a second variable to check if anything else that was not of source
defaulthad those fields in there.So the code was something like this:
So far it was simple, however what to do with the other core fields such as
StorageLocationandIsUsable, I could include them in the vars from the example above or have a third one for checking them but every time I played around the code started look less and less comfortable due to the short-circuiting nature of the language.This is a core validation, we cannot no matter what, allow it proceed without them, so duplicating it to use with both vars above didn't looked very nice.
To fix it, I used the original validation code and extracted only the parts that checked
pathand includedinputso that both could be later be evaluated against source beingdefaultor not:It works as expected and it doesn't mind if
pathorinputis provided when sendingdefaultas the source, because later the switch will completely ignore those fields and set a executable to justblender.The problem is that now that I check if
inputis being given a test is failing, this one:👍 It was a learning mistake, I thought that in theory since
Goinitialize it's variables such asstringto""it would also initialize a*stringto""and consider that beingnil, so""==nilbut that's not correct.Nope, I reverted my commits. I was going through a spiral that didn't seem to improve too much of the code, I'd also prefer to keep them explicit empty strings and document it somewhere that for source
defaultthe payload should be like that.I did attempted to have them with default values, but after running the generator, I still had pointers like you said.
Try not to mix
&&and||in the same expression without using parentheses.(A && B) || Cis quite different fromA && (B || C), and writingA && B || Cis somewhat ambiguous. I'm sure Go has a way to evaluate this (probably left-to-right), but since it's so easy to make a mistake here, I would always recommend parentheses.I think this is not due to the short-circuiting, but rather due to the approach of trying to put everything into one condition. Extracting this into a different function will make things considerably easier, because then you can do things like this (warning, I didn't check this, just wrote it here in this comment):
There is very little magic in Go, which is a good thing. It's a simple language, and it won't make assumptions for you.
👍
Got it, again another learning mistake of mine 😅 when they said that the language doesn't use parentheses I took it way too literally, should've tried it, but your solution below is much better.
I did some minor tweaks, the major one was switching the function for denial instead of acceptance to better match what we are doing in the if statement.
As we discussed earlier in Blender Chat, I've passed the logger and added it in case the payload doesn't contain any valid blender path source, so in this situation we log:
The above is an addition to what we already have in case a missing field is hit while using any valid blender path source:
I wouldn't necessarily write a function to suit where it's used. I'd write it to make it clear what it does, and to minimize the number of mental inversions you have to make.
if !isComplete(...)is fine,if isIncomplete(...)is also fine, so at the condition it doesn't really matter.Here we get into more confusing territory. Since
checkResultis using positive terminology, but the function is checking for the negative, in order to understand it you constantly have to flip between those two perspectives in order to understand the code. I'd recommend keeping the terminology consistent, and just check the positive. So it's not just about the number of negations, it's also about the different perspectives the reader has to have.Furthermore, that particular code can be shortened to
return !checkResult.IsUsable, and without the negation, that just becomesreturn checkResult.IsUsable.In such cases, try to include why Flamenco thinks the configuration is incomplete. As a design guideline, messages should guide the user towards a working solution. Just saying what is wrong doesn't help them.
Same goes for
logger.Warn().Msg("received an unexpected blender path source"): when you say something has an unexpected value, show that value. Otherwise you're creating a hard-to-debug situation.@ -339,0 +359,4 @@return false}logger.Warn().Msg("received an unexpected blender path source")This function could return an error instead of a boolean. That error can then not only be used to communicate what is still missing, but also cover this case. That'll avoid the need to pass a logger to the function.
Unfortunately Gitea does not support multiple comments for the same source line. So I'll reply to the previous one, even though at this line number there is now different code.
This should use
fmt.Errorf()instead.@ -339,0 +340,4 @@func isBlenderPathCheckResultIncomplete(checkResult api.BlenderPathCheckResult, logger zerolog.Logger) bool {switch checkResult.Source {case api.BlenderPathSourceDefault:if !checkResult.IsUsable {The check for
IsUsablecan be moved out of theswitchsince it's the same for everycaseanyway.👍 Thanks for the insight!
I can't deny the fact I did had to warp my head around while forcing the negative terminology, not something that I plan to repeat again, thanks you :)
I did kept the check for
isUsablein a if statement for better clarity as it now returns a nice message as suggested below.Got it, I wrote some better messages to guide the user, explaining why that particular payload was not accepted.
This is a demo for a few cases that I tested to show you how its looking now:
Done, sending an unknown source will receive this now:
The function was refactored as well, now it returns an error as suggested. I adjusted everything trying to follow some practices already present in code nearby, the only thing that I wasn't too sure is how to properly produce the returned error as I could do a
return fmt.Errorf("My message")orreturn errors.New("My message").I also have a question regarding pointers, would it be a good practice if I passed the address
&setupAssistantCfgto my function and worked with pointers inside of it? I became wondering a lot about this because we don't change anything on that function, we just check things and leave it alone.Nice, this is getting more hand-hold'y, I like it.
"configuration is invalid or incomplete" -- I think that can just become "configuration is incomplete". It's maybe slightly less correct, but it is simpler to read, and also conveys "this cannot be used as-is", which is the most important part.
👍
"sources cannot have the 'is_usable' field set to false" is, strictly speaking, cause for confusion. Because if it *cannot be set to
false, why is it complaining that it isfalse? Apparently it is possible for it to befalse. Change "cannot" to "should not", that'll correct things.👍
👍
As a rule of thumb: if you don't need to format a string, don't use the
fmtpackage. In that case you can actually declare those errors as package-level variables, and just return those.The caller can then use them to distinguish between various errors, if they want to, via
errors.Is(err, ErrConfigIncompleteUnknownSource).And maybe that name is a bit on the long/verbose side. Use your own judgement to see what's the best name, given the package context and the other errors.
Copies are generally cheap. Go (unfortunately) doesn't have 'const pointer' or 'const reference' concepts, so the easiest way to ensure that a function doesn't change the data held by the caller is to pass that data by value.
Things are improving well!
@ -271,2 +268,2 @@logger.Warn().Msg("setup assistant: configuration is incomplete, unable to accept")return sendAPIError(e, http.StatusBadRequest, "configuration is incomplete")if err := checkSetupAssistantConfig(setupAssistantCfg); err != nil {logger.Warn().Msg("setup assistant: configuration is invalid or incomplete, " + err.Error())The work cannot be done, and so this should be logged as an error, not a warning.
I usually log errors as a 'cause' field:
Alternatively, if you want to keep the error message as part of the log message, you could use
.Msgf(...)instead:%vis Go's generic "put the thing's value here", and should result in the value oferr.Error().@ -272,1 +268,3 @@return sendAPIError(e, http.StatusBadRequest, "configuration is incomplete")if err := checkSetupAssistantConfig(setupAssistantCfg); err != nil {logger.Warn().Msg("setup assistant: configuration is invalid or incomplete, " + err.Error())return sendAPIError(e, http.StatusBadRequest, "configuration is invalid or incomplete")It's fine to include the actual error message in the response as well, so that the caller knows what's wrong without checking the Manager log.
@ -339,0 +358,4 @@config.BlenderExecutable.Input == "" {return errors.New("'path' or 'input' fields must not be empty while using the 'input_path' or 'path_envvar' sources")}default:Add another
case "". That way the dynamically generated error below doesn't just show"unknown 'source' field value: "if thesourcefield is empty.Another solution would be to use the
%qformat specifier in the below-suggested call tofmt.Errorf(), but that would always put quotes around the value. If that error is then subsequently logged with something likelog.Error().Err(err).Msg("whatever"), those quotes will get escaped and things will be uglier than necessary. So that's why I think it's nicer to handle the empty string as a special case, and keep the non-empty-string case without quotes.@ -1539,6 +1539,8 @@ components:- path_envvar# The input path was used as-is, as it points to an existing binary.- input_path# The input path was not provided, use whatever blender is available.Add "…is available, without checking" to make it explicit that this will forego any checks on the path.
Thank you, I'm learning a ton in this PR 👍
I've tried my best to apply your feedback and I think it's looking better.
Here are some highlights of my changes:
There are now declared errors just below the imports, I tried to copy the style from other files.
I took some time to pick a name I thought it would fit and ended up with the prefix
ErrSetupConfig. It is still quite long but I think its fitting in the context of what it does, but please let know what you think.I really enjoyed
logger.Error().AnErr("cause", err).Msg("setup assistant: configuration is incomplete")It looks prettier than the inline method I was using and more concise with other Flamenco logs, below I'll show a demo.
The empty source case has been implemented, now it has its own
ErrSetupConfigEmptySourceand it will output this:This is for the dynamic error built with
fmt.Errorf():Sorry for the long silence. If you feel this PR is ready for re-review, please press the little 'request review' button next to my name in the top-right corner. That'll make the PR show up in my review queue again.
Things keep improving :)
The one thing that I'd like to see changed, is that right now the "Skip" option is only available when Blender is not found. I think it's better to simply always show that option. It'll not only solve the issue (of the Manager requiring Blender), but also open op to situations where Blender is found on the Manager (but not in a place that's usable on the Workers).
Hi, I updated the code to include the skip option on all scenarios.
I was going to copy and paste the code for that option but I decided to simplify our existing code by removing the condition in
so now there's only one
<fieldset>and inside of it evaluates as usual whatever is available for the user to pick, with the addition of our skip option at the end.This is how it looks like on a system that has Blender and one that doesn't
Approved!
Thanks for your work on this, it's going to make people's lives a little easier :)
I'll land the PR in a few different commits, as per Flamenco's commit guidelines.