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

Simplify installation of R development dependencies #1930

Merged
merged 2 commits into from Jan 12, 2021

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Dec 1, 2020

This removes the special script for installing the R dependencies needed for development and moves those to environment.yml. This is simpler to maintain and also faster because it removes the need to compile the R development dependencies.


Checklist for technical review

  • The pull request has a descriptive title that can be used as a one line summary in the changelog
  • The code is composed of functions of no more than 50 lines and uses meaningful names for variables and follows the style guide
  • Documentation is available

Automated checks pass, status can be seen below the pull request:

  • Circle/CI tests pass. If the tests are failing, click the Details link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • The documentation is building successfully on readthedocs and looks well formatted, click the Details link to see it.

- r-docopt
- r-lintr
- r-styler
- r-yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

that's great this removes the compilation stage of these packages - will the other packages (like akima, etc) need to be compiled still? How much of a hit we're taking from extra packages in the conda env and the possibility the env will now be harder to solve? (I am not expecting an answer to the last question since that really depends on conda weather, but just a precautionary thought) 🍺

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the other packages (at least the ones that are not pulled in as dependencies of these packages) will still need to be compiled. Indeed the environment will become slightly harder to solve. It would be possible to add many more R packages, because most of them are available on conda, but that would definitely make the environment harder to solve, especially when building the package from package/meta.yml.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it's a question of the lesser evil I guess

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

OK @bouweandela I fixed the conflict and I am approving it, let's watch the GA if there's any issues with env creation after merge, I haven't actually attempted env creation myself 👍

@bouweandela
Copy link
Member Author

@jvegasbsc Any comments on this?

Copy link
Contributor

@jvegreg jvegreg left a comment

Choose a reason for hiding this comment

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

Nice!

@jvegreg jvegreg merged commit 3898ad7 into master Jan 12, 2021
6 checks passed
@jvegreg jvegreg deleted the simplify_dev_install branch January 12, 2021 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants