Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add broadcasts for sequences #2971

Merged
merged 6 commits into from
Jul 15, 2024
Merged

Add broadcasts for sequences #2971

merged 6 commits into from
Jul 15, 2024

Conversation

joeyballentine
Copy link
Member

@joeyballentine joeyballentine commented Jul 1, 2024

Wanted to make this a draft to get some early feedback. There's a few things that still have to be done, mainly fixing the issue where it shows the previous broadcast value, which I have been unable to find even a shred of a lead at what's causing it.

Note: I decided to store the lengths in the outputNarrowing map under the "length" key. This is obviously not an ideal solution, but the alternative was duplicating all the same kind of code for storing it in a new map, and i felt it was similar enough to just throw it in there, at least for a proof of concept first.

TODO:

  • Expand autorun capability to all the generators
  • Remove generators from the cache after sending the broadcast (to avoid them resuming from the previous index)
  • Fix the stale broadcast issue
  • Probably more

image

@joeyballentine joeyballentine marked this pull request as ready for review July 2, 2024 02:55
@joeyballentine joeyballentine changed the title Add broadcasts for sequences (WIP) Add broadcasts for sequences Jul 2, 2024
@joeyballentine
Copy link
Member Author

joeyballentine commented Jul 13, 2024

Whats the status on this? Is there anything that needs to be actively addressed? If something is wrong, please tell me what i should change

@RunDevelopment
Copy link
Member

RunDevelopment commented Jul 14, 2024

Same as before, no?

  • Caching is either bugged or otherwise broken for iterators.
  • You don't know what the underlying issue is and just nuked the cache.

So please fix the underlying issue, because if we are running one iteration, that'll be a huge problem because it'll run nodes with side effects.

@joeyballentine joeyballentine marked this pull request as draft July 14, 2024 15:02
@joeyballentine
Copy link
Member Author

because if we are running one iteration, that'll be a huge problem because it'll run nodes with side effects.

That's not at all what's happening. The problem I mentioned is if you run the chain, stop it, then start again. It is not running an iteration when it auto runs. It's just running the node function, which returns the generator object. it seemed that keeping the same object in the cache treats it the same way as pausing, which is why I needed to dump the cache.

I'm not sure how you got that we're running an iteration from anything I said

@RunDevelopment
Copy link
Member

Oh. Well, this PR is about broadcasts, so I assumed that change had something to do with it... I assumed that you discovered this bug because of broadcasts (/run/individual) and then talked about stopping since it happens there too. But I guess it has nothing to do with broadcasts, and is only a fix for stopping iterators? Is that it?

@joeyballentine
Copy link
Member Author

Correct

@RunDevelopment
Copy link
Member

Alright, then it should be good.

@joeyballentine joeyballentine marked this pull request as ready for review July 15, 2024 20:17
@joeyballentine joeyballentine merged commit a2135a0 into main Jul 15, 2024
17 checks passed
@joeyballentine joeyballentine deleted the generator-broadcasts branch July 15, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants