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

Rebuild installation page #2028

Merged
merged 11 commits into from
May 26, 2022

Conversation

epassaro
Copy link
Member

@epassaro epassaro commented May 17, 2022

Description

A complete rebuild of the installation page.

Preview: https://tardis-sn.github.io/tardis/pull/2028/installation.html
Old version: https://tardis-sn.github.io/tardis/latest/installation.html

Motivation and context

The installation page required changes. It contains lots of redundant information.

How has this been tested?

  • Testing pipeline
  • Other

Examples

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • None of the above

Checklist

  • I have updated the documentation according to my changes
  • I have built the documentation by applying the build_docs label to this pull request (if you don't have enough privileges a reviewer will do it for you)
  • I have requested two reviewers for this pull request

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #2028 (8509b14) into master (0205add) will decrease coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2028      +/-   ##
==========================================
- Coverage   60.13%   59.88%   -0.26%     
==========================================
  Files          70       70              
  Lines        8108     8154      +46     
==========================================
+ Hits         4876     4883       +7     
- Misses       3232     3271      +39     
Impacted Files Coverage Δ
tardis/io/atom_data/base.py 81.81% <0.00%> (-7.90%) ⬇️
tardis/montecarlo/montecarlo_numba/base.py 29.75% <0.00%> (-2.07%) ⬇️
...dis/montecarlo/montecarlo_numba/numba_interface.py 35.17% <0.00%> (-1.04%) ⬇️
tardis/plasma/properties/atomic.py 55.73% <0.00%> (-0.45%) ⬇️
tardis/plasma/properties/continuum_processes.py 35.75% <0.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.


First, download the `environment definition file <https://raw.githubusercontent.com/tardis-sn/tardis/master/tardis_env3.yml>`_ from:
::
3. a. Non-developers can install the package with latest changes from upstream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want this? For reproducibility I thought lock files were only created for releases, so there may be unexpected breaking changes on the development version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand this one, can you explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referring to point 2 or 3.b?

Copy link
Contributor

Choose a reason for hiding this comment

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

3.a. the recommendation that users should install from git instead of conda-forge or a release tag.

Copy link
Member Author

@epassaro epassaro May 18, 2022

Choose a reason for hiding this comment

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

Ok, some considerations:

  • Installing directly the conda-forge package on a clean environment pulls all the dependencies required to run TARDIS. These dependencies are based on the meta.yaml file of the conda-forge/tardis-sn-feedstock repository and it's different to tardis_env3.yml. That's why I don't recommend using it (because solves dependencies before installation, diminishing reproducibility), but It should be possible to install this package safely once the dev env is already installed and using the --no-update-deps flag. I didn't think about this, but sounds good!. Should look like this:

    $ conda install tardis-sn -c conda-forge --no-deps
    
  • conda-forge package is currently not tested (requires Fix file paths in tests #1715). However, this is only necessary if you pull the package on an empty environment. Source code of the package is the same.

  • Currently, we don't have a way to maintain a single dependency file (I'm still thinking how to do it) and AFAIK no other organization has solved this problem yet.

  • Another way to install the latest release could be:

    $ pip install git+https://github.com/tardis-sn/tardis.git@latest
    

    But currently we don't have a latest tag. I've seen this in another packages, I assume they make two releases, one with the corresponding release tag and the other named latest. The latest tag is overwritten on every release cycle.

    Also, is possible to install the latest release just by calling the GitHub API:

    $ pip install "git+https://github.com/tardis-sn/tardis@$(curl -s https://api.github.com/repos/tardis-sn/tardis/releases/latest | jq -r .tag_name)"
    

docs/installation.rst Outdated Show resolved Hide resolved
@Rodot- Rodot- self-requested a review May 19, 2022 18:26
@epassaro epassaro marked this pull request as draft May 19, 2022 19:46
@epassaro epassaro marked this pull request as ready for review May 20, 2022 00:49
@epassaro
Copy link
Member Author

I think you were right @andrewfullard, I wasn't explaining my idea clearly. This new version should be better (I hope).

@andrewfullard andrewfullard merged commit 46a02a2 into tardis-sn:master May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants