ScienceBasedTargets / SBTi-finance-tool

This toolkit helps companies and financial institutions to assess the temperature alignment of current targets, commitments, and investment and lending portfolios, and to use this information to develop targets for official validation by the SBTi. See the wiki for a change log.
https://sciencebasedtargets.org/
MIT License
50 stars 42 forks source link

Issues performing .get() to CTA_FILE_URL from behind internal firewall #303

Open aleks-sch opened 1 year ago

aleks-sch commented 1 year ago

Our model currently uses v1.0.5 and we are struggling to update to v1.0.6+ because the requests.get() introduced in 2c9ca5d24b834db65910324abb2886e650b6045e to get the CTA_FILE_URL from behind internal firewall fails. We can go through a semi-lengthy exemption process but below changes will make it simpler and generally safer.

this issue to discuss some possible changes

mountainrambler commented 1 year ago

Hi and thanks for your input. I will look into the possbility of using JSON - if the SBTi has this possbility. A fallback will be introduced so that we check for a status code of 200, and if this fails we'll fetch the file included in the package instead. It might not be the very latest but I think it will be possible to manually load the file to that directory if the user can access it in some other way than through the tool. As for the verify parameter, I believe it is set to True by default.

aleks-sch commented 1 year ago

edited - hit send too soon!

thanks

JSON response would definitely be simpler for us to perform the request.get but i realise you would need to make a change to how the file is hosted and served!

shall I knock up a PR to add the targets.xlsx file to the GH repo and read from local if the request.get fails? I would just put the current file into https://github.com/ScienceBasedTargets/SBTi-finance-tool/tree/main/SBTi/data?

manually load the file to that directory if the user can access it in some other way than through the tool

suggestions for where could get user to load the file to?

as for verify param: our company decrypts our HTTPS traffic and would need verify to point to our trusted root cert. And I have realised that I may be able to just use the REQUESTS_CA_BUNDLE env var to point to this certificate >> from https://requests.readthedocs.io/en/latest/user/advanced/#ssl-cert-verification

image
mountainrambler commented 1 year ago

Well here's some good news. There will soon be a new api with the CTA delivered in JSON format. I don't have a date yet, but the code to update the tool is being prepared. The change will include a test for status == 200 and it it fails we'll read the file target-dashbard.xlsx from the directory STBi/inputs. And as for verify, is it enough for you if we add verify=True to the .get()? That's no problem from my side.

aleks-sch commented 1 year ago

thats great thanks - shall we leave this issue open until the API and check if status == 200 PR has been opened so can link the PR and issue?

And as for verify, is it enough for you if we add verify=True to the .get()? That's no problem from my side.

We should be able to just use the REQUESTS_CA_BUNDLE env var in our model execution environment thanks, I should have found that earlier!

mountainrambler commented 1 year ago

Yes, let's leave it open until the API is ready. But I will add the status == 200 and perhaps leave an option to read the CTA file from the directory "data" that is created when running the tool in Colab. (If you run it in your own Jupyter environment you should be able to see the CTA file anyway.)

aleks-sch commented 1 year ago

thanks for changes in #306 and 8426dc83a5bea6c0646435bf967d3c2daf505ea5 to add local CTA file backup.

unfort we are still facing exceptions so wrapped the requests.get in try/except in #308.

your review plz?