-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature python3 support lsh tweaks #364
Feature python3 support lsh tweaks #364
Conversation
we were cloning from source for a good reason, I'd need to revisit notes
removed tox file, no reason to support python2 in this project.
ok, I see there is an issue unpickling py2 objects in py3: requests-cache/requests-cache#83 I'll see if I can't convert it |
problem can be replicated with this:
hrm, I can get the value to unpickle in python 3 by changing it to:
but there are so many broken references after that. we should think about rebuilding the cache. I'll change the cache db path to include the version of python or leave unchanged if python 2 |
ha - er. the moment I push this change it's probably going to thrash iiif once it gets past the app tests and I'm signing off in a moment. Attaching the change as a patch. |
Nice work @lsh-0 . The |
We could boot So I think I'll turn on |
Nope, won't work as when we switch back the URL will be different. |
Tweak
|
that would definitely slow the huge number of requests made to iiif, but each article still does N requests, so it would be a steady stream instead of a waterfall. I'll do as you suggest though and keep an eye on the iiif server. We do have a ticket open somewhere to ensure iiif can survive a backfill |
- we're going to be stuck with it from hereonout, so lets not suffix it with 'py3' reduced the number of processes to use while generating article-json so we don't flood iiif. this change should be reverted once cache is rebuilt.
ok - dropped the number of processes to use down to |
ah - |
cool - it's going through the corpus now and I can see all the cache misses |
(would love to know where this is originating from:)
afaict |
https://elifesciences.atlassian.net/browse/ELPP-2973 is the ticket. |
The warning is from https://github.com/elifesciences/elife-tools/blob/develop/elifetools/parseJATS.py#L15
should be
I believe it is like this after the BeautifulSoup version used was incremented. I'll see if it works and arrange a PR on the elife-tools project. |
updating requirements.txt to clone elifetools to an editable repo and changing that line (which I thought was still valid anyway) to what they requested didn't suppress the warning for me. Maybe I did something wrong. |
build failed with:
investigating |
this failure:
|
@giorgiosironi , is there a timeout on the builds? I see this |
Yes, there is a default timeout of 2 hours. I'll override this. |
elifesciences/elife-tools#256 in elife-tools will use |
excellent, thanks both. I'll integrate your change @gnott and push it up to trigger the rebuild. |
this should suppress the warning about the parser being used
... ok.
the first and most important point: I have the tests running for python3, so thanks to @seanwiseman for all the hard work.
but also:
some controversial edits:
import src.foo.bar as bar
really irk me. I've replaced them withfrom src.foo import bar
type imports.src
is technically a package, it's not really used as such in the application. The exception is in the tests when they need to 'reach back' and reference modules in the parent directory.backportedmock
in favour of python3's builtinmock.patch
paths were behaving unusually and causing some really strange bugs except in one instance, which is unsettling: https://github.com/elifesciences/bot-lax-adaptor/compare/feature-python3-support...feature-python3-support-lsh-tweaks?expand=1#diff-5e0b291ba4f1474709fc8d4bcf4805d7R357I haven't gone through the original PR too closely yet.