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

Update installation instructions #720

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jun 19, 2023

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

  • recommends the libmamba solver when installing via conda
  • corrects the dependency arguments (or adds them) when using pip
  • recommends adding the main repo as remote upstream and fetches the tags from there

How to review

  • Read the diff and note that the CI checks all pass.
  • Build the documentation and look at the installation page.

PR checklist

* Recommend libmamba solver
* Correct dependency arguments for pip
* Recommend adding the main repo as remote upstream
@glatterf42 glatterf42 added enh New features & functionality docs Documentation labels Jun 19, 2023
@glatterf42 glatterf42 self-assigned this Jun 19, 2023
@glatterf42 glatterf42 linked an issue Jun 19, 2023 that may be closed by this pull request
@glatterf42
Copy link
Member Author

@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
Copy link

codecov bot commented Jun 19, 2023

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
Copy link
Member

khaeru commented Jun 19, 2023

@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.

INSTALL.rst Outdated Show resolved Hide resolved
@khaeru khaeru merged commit 3cd004a into iiasa:main Jun 21, 2023
@khaeru
Copy link
Member

khaeru commented Jun 21, 2023

Thanks @glatterf42! 🚀

@glatterf42 glatterf42 deleted the update/install-instructions branch June 21, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include mamba solver for conda in installation instructions Update/expand installation instructions
2 participants