Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix PyPy compatibility in adbapi init #4174

Closed
wants to merge 5 commits into from

Conversation

Valodim
Copy link
Contributor

@Valodim Valodim commented Nov 12, 2018

Following up to #2760 (and with #3098 fixed), this PR adds the simple required fix for the database initialization with adbapi to be able to run on pypy again.

I also added pypy35 to the tox and travis configs - let's see if that works :)

Signed-Off-By: Vincent Breitmoser look@my.amazin.horse

@Valodim Valodim force-pushed the pypy-adbapi branch 2 times, most recently from bd4702b to e6433f9 Compare November 12, 2018 12:24
@Valodim
Copy link
Contributor Author

Valodim commented Nov 12, 2018

Looks like the pypy35 test went through equally well as the python35 one.

But pypy35-postgres caused a bunch of errors, mostly "too many connections" on the psql server? The pypy tests also ran significantly longer than the py3 ones, not sure what to make of that :) On the plus side though, setting up tox for pypy was straightforward and worked almost right out of the box, yay

@Valodim Valodim mentioned this pull request Nov 12, 2018
@luminoso
Copy link

luminoso commented Nov 12, 2018

pypy tests also ran significantly longer than the py3 ones

I think this is expected due to JIT compilations. However, it should run faster after that

@richvdh
Copy link
Member

richvdh commented Nov 12, 2018

please see CONTRIBUTING.rst - this needs basing on develop, and needs sign-off.

@Valodim Valodim force-pushed the pypy-adbapi branch 2 times, most recently from a0bf945 to cd341c0 Compare November 12, 2018 15:09
@Valodim Valodim changed the base branch from master to develop November 12, 2018 15:12
@Valodim Valodim force-pushed the pypy-adbapi branch 2 times, most recently from b6054b2 to 73cf838 Compare November 13, 2018 13:45
@Valodim
Copy link
Contributor Author

Valodim commented Nov 13, 2018

Well. The test runs work now, but the pypy-postgres run takes a whoopin' 43 minutes for some reason. Wonder what the problem there is... anyways, I'll leave this here for now :)

.travis.yml Outdated
@@ -57,6 +57,19 @@ matrix:
services:
- postgresql

- python: 3.6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're excluding python 3.6 deliberately: we get four workers on Travis, so this one just slows things down pointlessly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, sorry. I think this one snuck during the rebase, didn't mean to add it

@richvdh
Copy link
Member

richvdh commented Nov 14, 2018

the pypy-postgres run takes a whoopin' 43 minutes

Well, that sounds like a show-stopper :-p

@Valodim
Copy link
Contributor Author

Valodim commented Nov 14, 2018

Sure does. Even the regular test takes 2.5 times as long :(

There's three ways from here:

  • figure out why it's slow and fix it :) heh. maybe if someone comes along who knows a lot about pypy
  • keep only the pypy-sqlite test around. it's not prohibitively slow but should still catch most pypy breakage
  • comment out the pypy tests in the CI pipeline, keeping pypy support in its current stepmotherly state. given that pypy's primary selling point is performance, but it's slower by a factor two in these (inadequate) tests, this is still a reasonable option

@luminoso
Copy link

@Valodim give it a try running the pypy tests with "--jit off" to check if JIT compilations are the ones causing the tests taking so long

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't really take this while it makes the CI runs take over 45 minutes. I suggest you remove the postgres job from travis.yml, and we can investigate why it is so slow elsewhere.

@luminoso
Copy link

Any news about this?

@richvdh
Copy link
Member

richvdh commented May 10, 2019

seems to have stalled, so closing for now. We can reopen it if anyone wants to continue work.

@richvdh richvdh closed this May 10, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants