-
Notifications
You must be signed in to change notification settings - Fork 10
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] Making UI simpler and an OOP refactor #114
Conversation
Desc. the new approach is so much intuitive and user friendly that it has become super easy now for the end-user to use it
|
||
Usage:: | ||
class NodesResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when instantiating a class without a complex parent, you should inherit from object
, i.e.:
NodesResponse(object):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the review! I didn't know this fact, and I learned it today. I'll update the PR.
pyobis/obisutils.py
Outdated
Handles technical details of sending GET request to the API | ||
""" | ||
headers = { | ||
"User-Agent": "Mozilla/5.0 (X11; CrOS x86_64 12871.102.0) AppleWebKit/537.36 (KHTML, like Gecko)\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might consider setting a user-agent string that specifies pyobis instead of this one. I assume this was copied from a browser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a browser user agent string, but I don't know if there are any UA detection tool into OBIS API. If it would be the case then, any other user agent string might work slower than the browser user strings because that might be used to put rate limits on bot networks.
And, I don't know why but the server response time is too high today for me. This request https://api.obis.org/v3/occurrence?taxonid=2&offset=0&mof=False&size=1 is taking 10.69 s waiting for server response while only 1.63 ms for content download. This is something unusual. I tried clearing DNS Cache, changing IPs, also tried VPNs, but the time taken is same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is something related to OBIS performing internal searches for all records and returning only the specified user size. This could be a critical bottleneck before pyobis being performant, but is a backend issue (something as a high-level design problem). I tested taxonid
s with lesser records footprint and the server wait time was nearly equal to content download time for a similar number of records. I analysed many requests for different number of records and found that the server wait time was a significant portion from 50% to taking nearly 95% of the total time for higher taxons. Therefore I came to the conclusion that for most users, more than network speed, server response time is a bigger bottleneck.
Interestingly, I have started playing with Request Headers and I find that some minor tweaks can increase performance by more than 25%. headers = {
"User-Agent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko)\
Chrome/107.0.0.0 Safari/537.36 Edg/107.0.1418.52",
"Accept-Encoding": "gzip, deflate, br",
"Host": "api.obis.org",
"Connection": "keep-alive",
} And found that it increased speed by more than 27%. |
Overview
This PR is huge and I'm really sorry for that.
Changes introduced
Thanks!