-
Notifications
You must be signed in to change notification settings - Fork 75
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
Pass keyword arguments to TDSCatalog object #303
base: main
Are you sure you want to change the base?
Conversation
…ssed into session_manager.
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.
Thanks for the contribution! And apologies on the delay in responding. Just a couple minor things here and this can go in.
@@ -276,9 +276,10 @@ def __init__(self, catalog_url): | |||
|
|||
""" | |||
self.session = session_manager.create_session() | |||
session_manager.set_session_options(**kwargs) |
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.
I don't think this line is doing any good since it sets options after self.session
is created. I'm pretty sure it's only the get
line below that's doing any good.
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.
This line is needed for my proxy setup. I use this line to modify the HTTP headers on the request session. If you look inside http_util.py
, the set_session_options
function adds the keywords I need. Specifically the "User-Agent" parameter in the header.
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.
If that's the case, you really should call set_session_options()
before calling create_session()
. set_session_options
only affects subsequent calls to create_session
.
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.
Interesting... if I comment out set_session_options
after create_session()
my code fails from the proxy blocking the request. I expect this.
If I move the set_session_options
line before create_session()
my code fails from the proxy blocking the request on Line 282 self.session.get(catalog_url, **kwargs)
. The same failure if comment out set_session_options
. This is unexpected based on what you told me.
I'm not going to pretend like I understand what is going on. I just know the code in this PR is the only thing that works for my network.
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.
Can you show the relevant sample of code you're using with siphon? At least the code that creates a TDSCatalog
up to the point that fails? I feel like there's something I'm missing here... (You can add that in a stand-alone comment below since I think we're getting a little long-winded here)
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.
Just added another comment with some relevant code samples. Sorry it took so long, ran into some issues with cftime
package updating and manifesting another bug in pvlib
.
I made another repo that shows how I am using this PR. I am doing solar forecasting with PVLIB which uses Siphon to gather data. I had to modify PVLIB to pass additional arguments into Siphon in order to successfully get the request through my corporate proxy. I hope you can follow my readme when creating the test environment. If you don't install |
Description Of Changes
My corporate network does not allow for standard SSL connections to weather forecast servers. I found that by allowing for passing keywords into the TDSCatalog object and then further into the session manager, I could send the appropriate headers and option flags into urllib that allow the get request to succeed.
Checklist