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

Update to pyo3 v0.14 #65

Merged
merged 3 commits into from
Aug 10, 2021
Merged

Update to pyo3 v0.14 #65

merged 3 commits into from
Aug 10, 2021

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Aug 4, 2021

Update pyo3 to the latest version.

@sansyrox
Copy link
Member Author

sansyrox commented Aug 4, 2021

Hi @messense .

Trying to update to the latest version is giving me a weird error. Even though this is working fine on my machine, I am getting errors like could not find native static library python3.8, perhaps an -L flag is missing? .

Do you happen to know why this might be happening?

@messense
Copy link
Contributor

messense commented Aug 5, 2021

⚠ Warning: You're building a library without activating pyo3's extension-module feature. See https://pyo3.rs/v0.14.1/building_and_distribution.html#linking

Because of this?

@JackThomson2
Copy link
Contributor

We should also wait until the asyncio library publishes a release rather than depending on a git branch

@sansyrox
Copy link
Member Author

sansyrox commented Aug 5, 2021

@JackThomson2 , that is true. It is just a test PR atm and also, the builds are not being generated on the base branch.

I am in touch with the maintainer of pyo3-asyncio and he plans to release a stable version soon. So, we will only merge it once everything is stabilized there.

@messense
Copy link
Contributor

messense commented Aug 5, 2021

https://github.com/sansyrox/robyn/blob/0551c63fa6333e48c4d5b50611078b0e6729e887/.github/workflows/CI.yml#L73 should be changed to pip install --force-reinstall dist/robyn*.whl

https://github.com/sansyrox/robyn/blob/0551c63fa6333e48c4d5b50611078b0e6729e887/.github/workflows/CI.yml#L40
may need to be changed to pip install --force-reinstall dist/robyn*_universal2.whl

@sansyrox
Copy link
Member Author

sansyrox commented Aug 5, 2021

@messense , thank you for this. Is this specific to v0.14.* or this can make things work in 0.13.* versions as well?

@messense
Copy link
Contributor

messense commented Aug 5, 2021

@messense , thank you for this. Is this specific to v0.14.* or this can make things work in 0.13.* versions as well?

It's only related to pip's new dependency resolver so both should apply.

@sansyrox
Copy link
Member Author

sansyrox commented Aug 7, 2021

Thank you @messense. It is now building on windows! 🥳

@sansyrox
Copy link
Member Author

sansyrox commented Aug 7, 2021

The stable version of the above library will be done by tomorrow. Let's try to make a release before 15th.

@sansyrox
Copy link
Member Author

sansyrox commented Aug 8, 2021

@JackThomson2 , I was trying to migrate to v0.14, but, I am having some difficulties setting the event loop. Can you have a look, if you understand it?

@ShadowJonathan
Copy link

ShadowJonathan commented Aug 8, 2021

@sansyrox could you maybe replace the Cargo.toml pyo3-asyncio section with the following;

pyo3-asyncio = { git = "https://github.com/awestlake87/pyo3-asyncio", rev = "dd6f2cc7838a2990ed15447ff9ef9f93fa51423d", features = ["attributes", "tokio-runtime"] }

awestlake87/pyo3-asyncio#30 is a bit in very active development, and so i recommend following and specializing exact commits instead of a branch, for reproducability's sake, because if you track a branch, rust might switch between grabbing the latest HEAD for that branch, and using the exact revision as denoted in Cargo.lock then and there.

This could help some with the problems you're having, or at least help with clearing up if the latest of the branch still has problems.

(https://github.com/awestlake87/pyo3-asyncio/pull/30/commits shows the commit hashes, time goes from old to new downwards, so always pick the last one in the list.)

(Pinging @awestlake87 here as well)

Edit: Yeah, your lock file is using commit d800b90 (#30), which is not the latest (in terms of code changes)

@sansyrox
Copy link
Member Author

sansyrox commented Aug 8, 2021

Hey @ShadowJonathan . Thank you for this. I have updated it in the latest commit.

@sansyrox could you maybe replace the Cargo.toml pyo3-asyncio section with the following;

pyo3-asyncio = { git = "https://github.com/awestlake87/pyo3-asyncio", rev = "dd6f2cc7838a2990ed15447ff9ef9f93fa51423d", features = ["attributes", "tokio-runtime"] }

awestlake87/pyo3-asyncio#30 is a bit in very active development, and so i recommend following and specializing exact commits instead of a branch, for reproducability's sake, because if you track a branch, rust might switch between grabbing the latest HEAD for that branch, and using the exact revision as denoted in Cargo.lock then and there.

@awestlake87
Copy link
Contributor

I opened PR #70 which appears to fix this. Sorry it's not a cleaner fix, it was a bit more complicated than I anticipated.

Essentially, since we changed over to using get_running_loop, the conversions no longer have access to the 'global' python event loop. Normally this is addressed automatically by pyo3_asyncio::tokio::run_until_complete or pyo3_asyncio::tokio::run with task local data. I fixed this by using pyo3_asyncio::tokio::scope_local to store the event loop in task-local data during the index route call.

@ShadowJonathan
Copy link

I fixed this by using pyo3_asyncio::tokio::scope_local to store the event loop in task-local data during the index route call.

Any reason why this cant just be scope?

@awestlake87
Copy link
Contributor

awestlake87 commented Aug 8, 2021

Yeah, it's not super clear from that snippet, but the routers variable contains some !Send data, so I needed to use the !Send variant of scope

sansyrox and others added 3 commits August 10, 2021 14:11
test with extension module

Check pipeline

Fix pyo3 pipeline

Try changing shell

Try removing deprecation

Update latest Cargo files
Co-authored-by: Andrew J Westlake <awestlake87@yahoo.com>
Update Cargo.toml to point to head
@sansyrox
Copy link
Member Author

Thank you, everyone!

@sansyrox sansyrox deleted the pyo3-update branch August 10, 2021 09:37
This was referenced Aug 11, 2021
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.

5 participants