iiasa / message_ix

The integrated assessment and energy systems model MESSAGEix
https://docs.messageix.org
Apache License 2.0
119 stars 153 forks source link

Update installation instructions #720

Closed glatterf42 closed 1 year ago

glatterf42 commented 1 year ago

Closes #698 and #718. According to the notes I gathered during recent installation/CI runs, this PR

How to review

PR checklist

glatterf42 commented 1 year ago

@khaeru Do you think we should mention not installing jpype version 1.4.1 in the installation instructions? I was thinking of adding that to the 'known issues', but then I saw that you removed the temporary workaround in iiasa/ixmp#463, so I don't know if this is still relevant?

codecov[bot] commented 1 year ago

Codecov Report

Merging #720 (1b97c02) into main (6dee23a) will not change coverage. The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #720   +/-   ##
=====================================
  Coverage   94.4%   94.4%           
=====================================
  Files         43      43           
  Lines       3448    3448           
=====================================
  Hits        3257    3257           
  Misses       191     191           
khaeru commented 1 year ago

@khaeru Do you think we should mention not installing jpype version 1.4.1 in the installation instructions? I was thinking of adding that to the 'known issues', but then I saw that you removed the temporary workaround in iiasa/ixmp#463, so I don't know if this is still relevant?

It appears to have resolved itself. Both the ixmp and message-ix "pytest" CI workflows appear to be using JPype1 v1.4.1, and test_del_ts is passing. We don't have a concrete understanding of what was causing the issue: we only know that it appeared around the time JPype1 v1.4.1 appeared.

tl;dr—I'd agree with your judgement that we don't need to specifically mention it in the install instructions.

khaeru commented 1 year ago

Thanks @glatterf42! 🚀