Skip to content

Add C compiler to conda environments / recipes #1141

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

Merged
merged 4 commits into from
May 11, 2023

Conversation

charlesbluca
Copy link
Collaborator

Adds c-compiler to our conda environment files and compiler('c') to our conda nightly recipe, so that the package can be successfully tested/built on systems that don't have cc pre-installed; haven't placed much thought into what version we should pin to (if any).

@@ -22,6 +22,7 @@ build:

requirements:
build:
- {{ compiler('c') }} =11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I set this pin somewhat arbitrarily, but it seems like GCC 11 successfully built the shared object locally for me

Copy link
Contributor

Choose a reason for hiding this comment

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

Would do this by adding a conda_build_config.yaml file with c_compiler_version specified (like so)

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Merging #1141 (7cdb597) into main (ab7340b) will increase coverage by 2.45%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #1141      +/-   ##
==========================================
+ Coverage   79.08%   81.54%   +2.45%     
==========================================
  Files          78       78              
  Lines        4395     4395              
  Branches      797      797              
==========================================
+ Hits         3476     3584     +108     
+ Misses        743      631     -112     
- Partials      176      180       +4     

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Charles! 🙏

Added a few suggestions below

@@ -22,6 +22,7 @@ build:

requirements:
build:
- {{ compiler('c') }} =11
Copy link
Contributor

Choose a reason for hiding this comment

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

Would do this by adding a conda_build_config.yaml file with c_compiler_version specified (like so)

@@ -22,6 +22,7 @@ build:

requirements:
build:
- {{ compiler('c') }} =11
- {{ compiler('rust') }} >=1.65.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Also would move this to conda_build_config.yaml and use rust_compiler_version (similar to c_compiler_version) above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we still able to specify a lower bound like thing with something like

rust_compiler_version:
    - '>=1.65'

Tried this locally and things seemed to work fine, but not sure if this is just because the compiler version is invalid and getting ignored

Copy link
Contributor

Choose a reason for hiding this comment

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

Think that would work

Though it could also make sense to pick a version for reproducibility reasons

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair, I'll go with the latest version being picked up for now (1.69) and we can revisit if there are any issues around that.

Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Defer to suggestion from @jakirkham for final approval. Changes lgtm!

@@ -3,6 +3,7 @@ channels:
- conda-forge
- nodefaults
dependencies:
- c-compiler
Copy link
Collaborator Author

@charlesbluca charlesbluca May 10, 2023

Choose a reason for hiding this comment

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

Despite this being tightly pinned in conda builds, I don't think there's an easy way to pin gcc in the conda environments? c-compiler brings in different compiler packages based on platform, and without the use of conditional selectors I don't think there's a way to pin all of those compiler packages in a single conda environment without causing conflict on at least one platform.

cc @jakirkham if you know of a way to achieve this, happy to punt if not

Copy link
Contributor

Choose a reason for hiding this comment

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

Could have different environment files per platform

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that makes sense - I think that for the sake of minimizing environment files to manage, I'm going to leave things as is since CI seems to be passing with GCC left unpinned. If we do run into failures at some point due to leaving it this way, I'd imagine that platform-specific environment files is the best workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense. Would probably do the same thing

Happy to revisit if it becomes more relevant

@charlesbluca charlesbluca merged commit f8b7d05 into dask-contrib:main May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants