Use 'int64' instead of non-portable 'long' #123
Conversation
unfortunately 'long' does not mean the same thing in different osses/ compilers. On windows long will only be 32 bit (int32). Setting width explicitly is more portable.
|
Thx @k-dominik, du bist klasse! |
.github/workflows/conda-build.yml
Outdated
| @@ -22,7 +25,7 @@ jobs: | |||
| with: | |||
| auto-activate-base: true | |||
| activate-environment: "" | |||
| channels: local,conda-forge,defaults | |||
| channels: local,conda-forge | |||
There was a problem hiding this comment.
looks like we need to add miniconda-version: "latest" to this action to make it work on macos
| @@ -10,8 +10,11 @@ on: | |||
|
|
|||
There was a problem hiding this comment.
looks like we need to replace RELEASE_VERSION=$(date +%Y%m%d%H%M%S) by something that runs on windows-latest runner
There was a problem hiding this comment.
good catch - also - it looks like the versioning scheme has recently been changed to calver, but no release has been done yet. I would strongly argue against date-based versioning. Meaningful pinning of this package for people that use it as a library is basically impossible with calver. Also it does not allow you to reason about versions in any meaningful way.
Of course this is also not automatically given with semver, and I have been fooled by relying on it as well. But most of the time I find it more useful than not. Only for end-user softwares (ubuntu) I can see a benefit in using calver. So... While orthogonal to this, would you be willing to reconsider?
There was a problem hiding this comment.
completely agree! It should be semver everywhere
|
I've added a commit reverting back to the original versioning scheme, also reading the metadata from setup.py -> no need for the environment variable shenanigans. As it stands this repo would have produced packages and code with versions out of sync (conda package calver, code would rely on init.py...) |
| @@ -1,6 +1,8 @@ | |||
| {% set setup_py_data = load_setup_py_data() %} | |||
| @@ -10,8 +10,11 @@ on: | |||
|
|
|||
There was a problem hiding this comment.
completely agree! It should be semver everywhere
miniconda pulls in defaults channel which most people like to avoid for licensing reasons. Also sets channel priority to strict and removes multiple channel source definitions for more reproducible environments.
|
I can fix the |
|
okay weird, the numpy<2 bin should not be necessary (I'm investigating...) - could you trigger the pipeline in any case to run please @wolny |
on win there are permission issues if delete_on_close is false
strict solution of env currently not possible on linux
k-dominik
force-pushed
the
fix-win-compat-long
branch
from
4fdd701 to
ddcd2a3
Compare
|
Okay, removed the numpy<2 pin as I could at least track down the environment solving problem to the |
|
this is good to go imo. |
|
amazing that you've made the build work on all 3 OSes! Thanks again for this PR 🚀 |
Unfortunately 'long' does not mean the same thing in different osses/compilers. On windows long will only be 32 bit (int32). Setting width explicitly is more portable.
The default dtype for in in numpy on windows is int32, but for pytorch int64... anyway. Cross-entropy loss will fail with 'long' on windows.
Bonus: Added CI to test win, macos ❤️