Skip to content
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

Refactor to use ixmp4 #766

Merged
merged 23 commits into from
Aug 23, 2023
Merged

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Aug 11, 2023

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

This PR refactors the ScSeAuth class to not retrieve the token for connecting to a Scenario-Explorer database directly but instead use the default-auth from the ixmp4 package.

Because ixmp4 0.2.0 only supports python 3.10, older Python versions are remove and 3.11-tests are currently disabled.

There are two issues that make this new dependency a bit more elaborate than necessary:

  • users may have local (non-standard) "creds" files, so these credentials are used instead of the ixmp4 default
  • users with standard pyam creds files will see a deprecation-warning to log in via ixmp4
  • we have to keep backward-compatibility with the legacy Scenario Explorer databases, so we need to keep the anonymous token

@danielhuppmann danielhuppmann self-assigned this Aug 11, 2023
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #766 (5772adc) into main (e70a0b7) will increase coverage by 0.2%.
The diff coverage is 70.9%.

@@           Coverage Diff           @@
##            main    #766     +/-   ##
=======================================
+ Coverage   94.4%   94.6%   +0.2%     
=======================================
  Files         62      62             
  Lines       6132    6107     -25     
=======================================
- Hits        5789    5779     -10     
+ Misses       343     328     -15     
Files Changed Coverage Δ
pyam/iiasa.py 89.8% <65.3%> (+2.6%) ⬆️
pyam/logging.py 65.0% <100.0%> (+0.8%) ⬆️
tests/test_iiasa.py 99.4% <100.0%> (+1.5%) ⬆️
tests/test_io.py 100.0% <100.0%> (+1.4%) ⬆️

@phackstock
Copy link
Contributor

Quick question about future logins for non-ixmp4 projects. Say I need to read some data from the private NGFS phase 4 internal scenario Explorer after this change is live on pyam. Can I do that then?

@danielhuppmann
Copy link
Member Author

Quick question about future logins for non-ixmp4 projects. Say I need to read some data from the private NGFS phase 4 internal scenario Explorer after this change is live on pyam. Can I do that then?

Good question, and please suggest how to make this clearer in the docs (if you think it's necessary).

Setting your credentials using ixmp4 login <username> (in a console) will work for Scenario-Explorer and ixmp4 Platform instances.

If you have already set your credentials via pyam v1.x and simply pull the latest version of pyam (and install ixmp4) and do nothing else, doing pyam.iiasa.Connection(...) will work, but you will get the following warning:

DeprecationWarning: Using a pyam-credentials file is deprecated and will be removed in future versions. 
Please run `ixmp4 login <username>` in a console and manually delete
the file '<your-home-directory>/.local/pyam/iiasa.yaml'.

@phackstock
Copy link
Contributor

Setting your credentials using ixmp4 login (in a console) will work for Scenario-Explorer and ixmp4 Platform instances.

Ah ok I see that's good to know. I'll suggest an update to the docs then.

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Some small comments below.
One question regarding the tests. Is there an explicit test for authentication using ixmp4? I tried and didn't see one but maybe it's all there anyway.

docs/api/iiasa.rst Outdated Show resolved Hide resolved
pyam/iiasa.py Show resolved Hide resolved
docs/api/iiasa.rst Show resolved Hide resolved
Co-authored-by: Philip Hackstock <20710924+phackstock@users.noreply.github.com>
@danielhuppmann danielhuppmann marked this pull request as ready for review August 22, 2023 15:01
@danielhuppmann
Copy link
Member Author

Is there an explicit test for authentication using ixmp4?

@meksor added several tests in ixmp4.

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, ready to be merged.

@danielhuppmann danielhuppmann merged commit 525ede9 into IAMconsortium:main Aug 23, 2023
@danielhuppmann danielhuppmann deleted the ixmp4/auth branch August 23, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants