Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sync some features with anaconda's recipe #318
Sync some features with anaconda's recipe #318
Changes from 52 commits
6facae4
63aa7dd
81363c0
e8d6740
e1f50ac
9fcb3a7
ca90fa0
ff040de
a2694a9
6074128
407a78d
393d0d5
0f9b587
4feaee5
0fe0ba4
14ca3d2
eaaae74
e19af70
939dae1
af73dbe
65bcd3b
a26ede2
03b2fc7
f3bfe5f
a811bb2
bd450c9
6f49c62
10bfd83
46f4e8e
c1c1e6c
3a34b59
e19e11c
5f19e7f
f6bbd00
9864e70
a002741
226f526
ad37dec
59af084
78b5aca
db810d0
a623264
e44b0d5
415a628
0a2094f
3889dee
982cadf
4683abe
e91c713
46d06c1
cdabb36
89d7354
8a92b36
8338fd7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to the PyTorch vendored copies of these when using the system copies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ignores them: pybind11 eigen
Although in the case of eigen it will use the vendored one if it doesn't find the system one, rather than erroring, which can mean accidental vendoring if you don't look at the logs carefully, which isn't great IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a patch to prevent this for mkl but I thought it best to look at the blas/openmp stuff closer and maybe address in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you ok with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if
CPU_COUNT
is undefined or empty string?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What CI provider do things run on for anaconda? I think it might need to become specific to that, and not in the overall
else:
clause here. Our OSX builds on azure work are successful withing the 6h limit, but seem to fail with this PR. This is a likely culprit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's AWS. Happy to just leave it as-is. I think originally we had it maxed at four cores and then increased it to CPU_COUNT-1 because of common practice and a desire to speed things up rather than concrete technical reasons. We can leave this as-is and if we have issues our end solve them later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed our case in 6074128, so I have no objection to doing what helps you in the
else:
branch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have made this a default in the else branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that
CPU_COUNT
is a passthrough environment variableSo one can set this in their CI scripts to the intended value and conda-build will use it. This is what conda-forge does
Instead of adding this to the build recipe, would recommend doing this in Anaconda's CI scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPU_COUNT
isn't being set here. If it's set (somewhere else, like in CI as you suggest) then it'll be used to setMAX_JOBS
. If it's not set, this expression will evaluate to 1There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we sync these flags across calls and Windows? Seems they vary a bit
Idk whether it is worth it, but we could also consider using
script_env
to set most of these flags once and reuse them throughoutThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for windows we've got
%PYTHON% -m pip %PIP_ACTION% . --no-build-isolation --no-deps -vvv --no-clean
and unix
$PREFIX/bin/python -m pip install . --no-deps --no-build-isolation -vvv --no-clean
So the same AFAICS?
Keeping them here seems more in line with what people will expect given other feedstocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any further comments?