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

Installation updates to README and .github/workflows #637

Merged
merged 60 commits into from Aug 18, 2023

Conversation

ras44
Copy link
Collaborator

@ras44 ras44 commented Jul 12, 2023

Proposed changes

This work in progress addresses issues that seem to be arising during installation on cloud instances, in docker, and in google collab.

This PR should help to address #624, #609, #593, #533, #534, and #636 by updating README.md and also providing GitHub workflows that confirm build success status for conda-forge, conda-venv, PyPI, and source installations.

Types of changes

What types of changes does your code introduce to CausalML?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

This PR contains two elements:

  • modifications to the README.md

    • mention requirements for gcc and g++
    • hint for installing miniconda
    • removing $ from existing code sections to make copy-paste easier
  • new .github/workflows

    • these implement tests for various build configurations
    • not completed yet

Appreciate your feedback!

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2023

CLA assistant check
All committers have signed the CLA.

@ras44 ras44 marked this pull request as draft July 12, 2023 17:54
@ras44
Copy link
Collaborator Author

ras44 commented Jul 26, 2023

Hi @jeongyoonlee , I was thinking about your comment re the gcc, g++ dependency section in the README, and this looks like it would take care of the compiler dependency in conda across a variety of OSs:

conda install -c conda-forge cxx-compiler

So as you mentioned, if we can include the above for anything with conda, it probably makes the most sense to move the install g++ notes to the pip and build from source option. Thanks for your suggestion!

@ras44
Copy link
Collaborator Author

ras44 commented Jul 26, 2023

@jeongyoonlee This PR is ready for an initial review. A brief summary:

Copy link
Collaborator

@jeongyoonlee jeongyoonlee left a comment

Choose a reason for hiding this comment

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

LGTM. The error for the test-pypi-install.yml build should have been addressed by #640. Is your branch rebased with the latest master?

I'm not sure what's happening with conda installation tests, though.

@ras44
Copy link
Collaborator Author

ras44 commented Aug 7, 2023

Thanks @jeongyoonlee My branch includes #640, and is currently only 1 commit behind master. So I think the error for the PyPI install is still valid. If I understand things correctly, the error arises because it pulls the 0.14.0 release, which doesn't contain the most recent fixes that resolve the build issues:

image

Once a new release is pushed to PyPI, those installations should pass (since the build and tests currently passes).

The same thing might be happening for conda-forge- although the conda-forge install is taking a very long time to complete, so there might be another issue there.

@jeongyoonlee
Copy link
Collaborator

Sounds good. @ras44, why don't we merge this PR and include it in the next v0.14.1 release?

@jeongyoonlee jeongyoonlee marked this pull request as ready for review August 18, 2023 17:10
@jeongyoonlee jeongyoonlee merged commit b02b5ce into uber:master Aug 18, 2023
6 checks passed
jeongyoonlee added a commit that referenced this pull request Aug 24, 2023
jeongyoonlee added a commit that referenced this pull request Aug 25, 2023
jeongyoonlee added a commit that referenced this pull request Aug 25, 2023
vincewu51 pushed a commit that referenced this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants