Lightmatter / welkin-health

A Python wrapper of the Welkin Health API
https://welkin.readthedocs.io
Other
4 stars 3 forks source link

Allow concurrent use of welkin clients #67

Closed jdiaz5513 closed 1 year ago

jdiaz5513 commented 1 year ago

Without this patch any attempt to use the Welkin client concurrently will result in a file access error.

This is because of the temporary file created for auth: the shelf library makes no attempt to lock the file beforehand and cannot handle concurrent write access.

I added portalocker as a cross-platform way to obtain a lock (on a separate temp file) while accessing that DB file.

I've also added pytest-xdist to make sure the tests do not fail when run in parallel (they did before!).

There is one newly skipped test that fails intermittently (~1 out of 20 times). I'm not sure what to do about it, or if it is even safe to skip. We're trying to add parallelism to our own test suite so that particular issue may come to bite us anyway. It is interesting that it's only happening to a single test.

jdiaz5513 commented 1 year ago

I can't reproduce the failing test anymore after hundreds of test runs; looks like this is safe to merge!

edcohen08 commented 1 year ago

Great, thank you @jdiaz5513 !

edcohen08 commented 1 year ago

@jdiaz5513 can you run poetry add urllib3@1.26.12, it was upgraded with portalocker and is breaking our tests with vcr

jdiaz5513 commented 1 year ago

@edcohen08 sorry that took me a bit; let's see if that fixes the packaging!

edcohen08 commented 1 year ago

@edcohen08 sorry that took me a bit; let's see if that fixes the packaging! @jdiaz5513 also have been very slow to come back to this, can you check actually it seems like the entire project packages and lock was overwritten, maybe there was an environment clash? if you could just make sure you're using the poetry.lock from dev and then only adding portalocker

gone commented 1 year ago

is this good to merge? it looks like the changes are pretty small

samamorgan commented 1 year ago

is this good to merge? it looks like the changes are pretty small

Yep, I think it should be. Just needs a review and approval.