8000 synclink integration by madhur-tandon · Pull Request #1258 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content

synclink integration #1258

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 73 commits into from
Mar 27, 2023
Merged

synclink integration #1258

merged 73 commits into from
Mar 27, 2023

Conversation

madhur-tandon
Copy link
Member

Originally 81 errors / failed tests --> Now down to 32 errors / failed tests.

This is a step for moving towards WebWorkers eventually.
CC: @hoodmane @antocuni

@madhur-tandon madhur-tandon added the sprint issue has been pulled into current sprint and is actively being worked on label Mar 6, 2023
@hoodmane
Copy link
Contributor
hoodmane commented Mar 9, 2023

I think before we get any more working we need to:

  1. Add a same-thread mode to Synclink so that syncify works alright on the same thread
  2. Add some better solution for attribute assignment to a synclink proxy
  3. implement @antocuni's autosyncify suggestion.

This is what we need to keep the Python plugin API both sane and moderately similar to what already exists. Another option would be to break all the existing plugins and work on fixing them in a subsequent PR. Maybe this needs to go into a long-lived workers branch.

@hoodmane
Copy link
Contributor
hoodmane commented Mar 9, 2023

@antocuni we should probably have a meeting on worker migration...

@madhur-tandon
Copy link
Member Author

I think the last commit broke some tests in test_interpreter.py again, looking...

@madhur-tandon madhur-tandon force-pushed the synclink branch 6 times, most recently from f61e798 to 668db62 Compare March 21, 2023 15:48
@hoodmane
Copy link
Contributor

I'm sorry it's apparently called Remote:

import type { Remote as SynclinkProxy } from "synclink";

@madhur-tandon
Copy link
Member Author 
import type { Remote as SynclinkProxy } from "synclink";

This requires converting to an unknown first i.e. as unknown as SynclinkProxy<PyProxy>

I have changed it to any as of now and we can handle this in a follow up PR, what say?

@hoodmane
Copy link
Contributor

This requires converting to an unknown first i.e. as unknown as SynclinkProxy

This is surely because of a wrong type somewhere else. You shouldn't be saying x as SynclinkProxy<T> in just one spot, it needs to change in many spots...

we can handle this in a follow up PR, what say?

This will mean merging this PR with a fair number of wrong types: things that are typed as T but are actually SynclinkProxy<T>. I'm not enthusiastic about this. It's much better for the type to be any than wrong. But it's up to @antocuni what he prefers.

@madhur-tandon
Copy link
Member Author

It's much better for the type to be any than wrong.

Like I said, I changed it to any already...

@hoodmane
Copy link
Contributor

The issue is that _remote has the wrong type. I'll push a fix.

@hoodmane
Copy link
Contributor

Very weird that c68f550 broke a test... it shouldn't change runtime behavior.

@madhur-tandon
Copy link
Member Author

Very weird that c68f550 broke a test... it shouldn't change runtime behavior.

This is likely a flaky py-repl test. This PR fixes a lot of them, but some still manage to seep in at times.

@hoodmane
Copy link
Contributor

Thanks for letting me know @madhur-tandon!

I fixed the rest of the types locally. I'll push the fixes when github is back up.

antocuni
antocuni approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor
@antocuni antocuni left a comment

Choose a reason for hiding this comment

The reason wi 6D40 ll be displayed to describe this comment to others. Learn more.

wooo, I think we are very close to merge it!
I hope that the build will be green now

@madhur-tandon madhur-tandon merged commit c8f9f16 into main Mar 27, 2023
@madhur-tandon madhur-tandon deleted the synclink branch March 27, 2023 15:26
This was referenced Mar 27, 2023
Closed
Closed
Closed
9E88
Closed
@JeffersGlass JeffersGlass mentioned this pull request Mar 27, 2023
Closed
@antocuni antocuni mentioned this pull request Apr 5, 2023
Closed
@antocuni antocuni mentioned this pull request May 23, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WebReflection WebReflection WebReflection left review comments

@antocuni antocuni antocuni approved these changes

@JeffersGlass JeffersGlass JeffersGlass left review comments

@hoodmane hoodmane hoodmane left review comments

Assignees
No one assigned
Labels
sprint issue has been pulled into current sprint and is actively being worked on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@madhur-tandon @hoodmane @antocuni @WebReflection @JeffersGlass
0