search

cables-gl / cables_docs

cables documentation docs.cables.gl
https://cables.gl/docs/docs
45 stars 16 forks source link

Code Ops can be in broken "no libs" "no clone" state, aren't flagged as usedInProject. #790

Open asford opened 4 months ago

asford commented 4 months ago

Link to simple, reproducible example patch

https://cables.gl/edit/gsiQSf

Describe the bug

Custom ops can enter a broken state where:

This appears to be cause by a backend issue where the ops aren't flagged as usedInProject in getAllProjectOps operation, see trace from https://cables.gl/edit/clfK2f in thread below.

The error condition isn't related to the external libs, but is instead related to how ops are flagged as used in the patch in the editor.

Even after the op is edited to a valid code state with valid libs, the op is still broken.

How To Reproduce

This is a result of editing the op in the patch, potentially with error conditions on the save operation. I have not exactly determined a repro path, but have a patch with an op in the broken state.

I have tried the following, which don't repro the failure:

These all have expected error conditions and don't break the op.

I think there's a repro path where the op is in a broken state due a missing top-level reference, it's dropped from the patch field, and then maybe edited directly via the op description page to re-import the op. This happens when the op's definition (not just callbacks) reference an external library by name.

The existing repro is at:

https://cables.gl/edit/gsiQSf

BrokenImportsVisionSmoke references these libraries and is broken. If used alone, the libs aren't included. (op id: eb972e6e-c716-4f27-acc4-ca4342a5a82f)

WorkingImportsVisionSmoke is a manual copy of this op. It has the same code and references the same libs and works. (op id: 4dbfaf7b-9986-4f1a-bf45-35e303d96a9a)

The ops are otherwise identical, have the same code and include the same libs. BrokenImportsVisionSmoke isn't flagged as usedInProject when added to the patch, WorkingImportsVisionSmoke is flagged. Hopefully this could be used to look at the op or patch state in the cables backend to determine if there's a difference in the op state.

Platform

https://cables.gl/browser/r/666f3219267d3383293f0d8a

steam0r commented 4 months ago

hey,

thanks for the detailed report. for some reason your patch loads with id 9c53bc6a-4b69-4ec0-80c0-a5fea1bc8f87 and that does not exist. somewhere along the line cables seems to use the name to lookup the op, which then gives the op with the id eb972e6e-c716-4f27-acc4-ca4342a5a82f...which seems to be the right op.

Bildschirmfoto 2024-06-17 um 12 23 19

it would be really interesting if you could reproduce this, did you rename the op? did you edit it in the external editor? maybe change between dev.cables.gl and cables.gl during clone, rename, ...?

how are you doing this, for example Renaming the op to move between patches do you manually rename the op to another op with a new patchid?

this is really, really strange...

steam0r commented 4 months ago

btw i left the state of the patch as "broken" to have a case to test and get back to as you seem to have found a workaround

asford commented 4 months ago

Note, see thread below, this repro path is unverified and may not be accurate. It is possible to repro via other actions.

Ah, great. Thanks for the hint and quick investigation.

I was changing the op names quite a bit because the editor drops the op from the patch-field if-and-only-if you reload the patch page while the op is in a broken (i.e. can't successfully execute the constructor) state. I didn't realize (at that point) that the patch maintains the op reference and will re-load it if it's fixed via the external editor.

As some note for both of us, which I'll investigate later. I suspect the edit path I took would be...

  1. Code an Op (e.g. {name: "TestOp", uuid: 0}) and add to the patch. ( patch_ops = [ {name: "TestOp", uuid: 0} ])
  2. Edit the Op code so it can't init cleanly, reload patch so it's dropped from the patch editor but present in patch def. ( patch = [ {name: "TestOp", uuid: 0} ])
  3. Rename the Op via the Op documentation interface (e.g. {name: "TestOp", uuid: 0} -> {name: "BrokeOp", uuid: 0}). This leaves the patch state unknown, does it reference by uuid, name or both?
  4. Code a new Op under the old name and add to the patch. (e.g. {name: "TestOp", uuid: 1} {name: "BrokeOp", uuid: 0} still referenced in the patch.
  5. Delete {name: "BrokeOp", uuid: 0} via the Op documentation page.
  6. Now the patch (maybe) has in invalid reference to {name: "TestOp", uuid: 0}, which partially resolves by name. Enough to load, but with unloadable external libs and unclonable.

If this is a repro path, then I don't fully understand whether the issue is in the Op definition or in the patch definition. I generated this repro by cloning my broken working patch and moving the broken op via rename into this new patch, so I haven't verified if moving the broken op definition into a blank patch will propagate the issue.

If the broken state is in the patch definition, or in an interplay between patch and op defns, then moving to a clean patch would resolve the op issue. If the broken state in in the op definition, then moving to a clean patch won't resolve the op issue.

asford commented 4 months ago

how are you doing this, for example Renaming the op to move between patches do you manually rename the op to another op with a new patchid?

Yes, I used the standard op rename page. E.g.

https://cables.gl/op/rename?op=Ops.Patch.OLD_PATCH_ID.NAME&new=Ops.Patch.NEW_PATCH_ID.NAME

I was thrashing a bit, so this might have involved a deprecation rename.

https://cables.gl/op/rename?op=Ops.Patch.OLD_PATCH_ID.NAME&new=Ops.Patch.OLD_PATCH_ID.Deprecated.NAME

I don't use dev.cables.gl, so this is all within the standard cables.gl site.

steam0r commented 4 months ago

okay, cool, thanks again. this is something to look at. if you have more insights, please let me know :)

also: i assume uuid: 0 is a placeholder here and by Define an Op you mean creating it via code a new op and/or patch a new op...or are you doing something different?

steam0r commented 4 months ago

in general: ops are referenced by uuid, that rename does not copy the op but rename it and keep the uuid...somewhere along the line something changes the uuid though apparently and your "old patch" still has a reference to the old uuid and cables get's confused by trying to be clever and look up the op by name...or something like this :|

steam0r commented 4 months ago

thinking about it: what is the thing you are trying to archive by renaming the op into another patchop? a patchop lives in one patch only, you can copypaste it but then it will create a copy. if you rename a patchop into another patchop your old patch would be broken...if it used the patchop still. are you trying to MOVE the op by renaming? what usecase am i missing?

asford commented 4 months ago

thinking about it: what is the thing you are trying to archive by renaming the op into another patchop? a patchop lives in one patch only, you can copypaste it but then it will create a copy. if you rename a patchop into another patchop your old patch would be broken...if it used the patchop still. are you trying to MOVE the op by renaming? what usecase am i missing?

Sorry if this is unclear, these are code ops not subpatch ops.

I'm using the rename-as-move operation in a few places to move code ops between patches. In this example, I just moved the ops to another patch so I could tidy them into the clean repro patch linked above. I generated the repros in a different work-in-progress patch.

also: i assume uuid: 0 is a placeholder here and by Define an Op you mean creating it via code a new op and/or patch a new op...or are you doing something different?

Yes, updated the language slightly for clarity.

steam0r commented 4 months ago

hm, interesting, rename does not really "move" at all, it just renames...even with code ops you would be better off copypasting them, or cloning the patch (which will also clone patchops)

asford commented 4 months ago

I've just had another repro which didn't include any of the op-renaming business above, just standard development cycle. However, I did noticed that cables.gl had a gateway error and some UI/backend flakiness during the dev cycle where I saved this op. Perhaps this is a hint that there was some weird partial-save behavior? Not totally clear.

With some debugging, see detailed notes below, I have isolated this repo to the getAllProjectOps API (i.e. the https://cables.gl/api/doc/ops/project/ endpoint). This API is not setting the usedInProject flag for the code op, even though it's included in the patch. The op is then filtered from the used op list during initial patch load, and the op's libs aren't included in the patch page. Reloading the op, which triggers an op reload regardless of the getAllProjectOps usedInProject flag, successfully loads the op's libs.

I was able to eliminate the repro by cloning the patch, which results in both op and patch clones.

At this point, I believe this is due to a backend state issue, potentially related to either the op or the patch state. Because is repro is preserved when renaming the op into another patch, I believe that this may be related to the op. As I don't have access to the backend code I've reach the boundary of my abilities and can debug no further. 😺🤷‍♀️

The repro case is at: https://cables.gl/edit/clfK2f

The patch includes a code op SemiHolistic which is in the repro state:

I debugged the error, with detailed traces:

hm, interesting, rename does not really "move" at all, it just renames...even with code ops you would be better off copypasting them, or cloning the patch (which will also clone patchops)

🤷‍♀️ yes, there are a number of reasons to clone-vrs-move. Cloning is generally preferred, for semi-obvious reproducibility reasons. However in this case, I just wanted to move the op into the repo patch so I could unblock development in my dev path. Sometimes it's also useful to move an op into the user namespace so I have a template to use the op in different patches.

steam0r commented 4 months ago

we decided to not allow renaming patch-ops of one patch into another patch in the next release, this should be handled with cloning patches or copypasting ops...the other workflow would be a lot of work regarding permissions and everything...sorry...

asford commented 4 months ago

we decided to not allow renaming patch-ops of one patch into another patch in the next release, this should be handled with cloning patches or copypasting ops...the other workflow would be a lot of work regarding permissions and everything...sorry...

I don't really understand what you mean by patch-op here. Do you mean a coded op within a patch?

In that case, thanks for the heads up. This isn't breaking for this specific issue, but will cause issues in other ways that I can probably work around.

Is there any way I can help with this specific issue?

steam0r commented 4 months ago

hey,

to clarify a bit: what i mean with patch-ops are ops that only exist in one patch, they live in the "Ops.Patch." namespace and have the patchid (shortid) in their name (e.g. Ops.Patch.PclfK2f.SemiHolisticVision lives in patch clfK2f). once you clone a patch or copypaste this op into another patch, these ops get COPIED over to the new patch and get a new name with the new patchid. wether these are "coded" or "patched" does not matter in this case.

the permissions and usecases behind that are explained here:

https://cables.gl/docs/5_1_permissions/3_ops/ops

renaming an op will do exactly that. it renames the op but retains the id (opid, not the patchid in the name). all projects that use the op with the old name will now show the new name.

if you rename a patch-op it will keep it's old opId and still live in the old patch, as it was not copied and we do not change project structure when renaming ops (intentionally). in your special case you renamed a patch-op into another patch op and manually set the new patchid. this will make the renamed op work in the new patch (as the id matches) but the old project now is broken, since it contains a patch-op of another patch...which is not legal.

to share ops between projects you should use userops or teamops, or even just clone the patch or copypaste the patch-ops you need in your new project. this will keep the old project unchanged and working.

we don't want, and most of the time can't, change the structure of projects, since that might break patches. so actually moving a patch-op from one project to another will have to include the steps outlined above. the decision to "not allow" this basically just means that renaming an op the way you did will now give you warning and not allow this, so you should never end up in the state that this issue is about in the first place.

i hope this explains this a bit...and i hope i understood your usecase and questions correctly. thanks again for the detailed report on this!

asford commented 4 months ago

we don't want, and most of the time can't, change the structure of projects, since that might break patches. so actually moving a patch-op from one project to another will have to include the steps outlined above. the decision to "not allow" this basically just means that renaming an op the way you did will now give you warning and not allow this, so you should never end up in the state that this issue is about in the first place.

Right, I understand the iter-project copy/rename issues. The permissions issues make sense; forcing a clone rather than rename is the simplest and safest (though most restrictive) option here.

To be very clear, the bug outlined in this issue was not caused by an op rename between patches. I have successfully renamed ops between patches multiple times, and this has not caused this issue. I used a rename operation to cleanup the repro case outlined above, but the bug occurred before the inter-patch rename. Changing op rename behavior will not affect this issue.

I've updated the documentation in this bug, please let me know if it's unclear. This bug has repro-ed multiple times now within a single patch, without op renames within a patch or between patches. This issue may originate in some rename flows, but I have never observed this directly. The ops can be moved into a broken state by basic save flows alone without renames.

steam0r commented 4 months ago

I've updated the documentation in this bug, please let me know if it's unclear. The ops can be moved into a broken state by basic save flows alone without renames.

okay, i misunderstood this then and will take a look at it again, sorry

steam0r commented 4 months ago

okay, had some time to look into it again, the op that is actually used in the project has the id 02eedd07-58c7-477e-8deb-312e64d72a18. you can see this by focusing the op and pressing ctrl/cmd-p and type show op serialized the op with the name Ops.Patch.PclfK2f.SemiHolisticVision has id 9a4083ab-bd17-4b98-a00b-38fb3b5cb084 (which you can see on the api link that you posted (which actually, for some reason, has the op-name in it twice, both same id).

this id is also used when you add the op to the patch by pressing escape and typing holistic, then look at the new op with show op serialized again.

reloading the patch and searching for the id (cmd/ctrl-f 9a4083ab) actually does show that the the patch/id is NOT used in the patch and hence not marked as usedInProject...which seems correct.

i seem to remember fixing some stale cache issue a while back, i rebuilt caches and now your patch says it cant load the op with the id 02eedd07-58c7-477e-8deb-312e64d72a18, which is correct, as that op does not exist anymore...

i still would love to know how exactly you ended up in this situation. so if it ever happens again, or you find out more please let me know! i still think this has something to do with clone, rename, copy, delete...something...

thanks again for your patience and the detailed reports!