[minor] `nim cpp -r torch.nim` => `nimble test` ? (more standard) #6

timotheecour · 2018-10-08T18:56:14Z

No description provided.

sinkingsugar · 2018-10-09T08:18:05Z

@jwollen What do you think? We are hardly using nimble much lately. People is already reluctant towards nim, conda is a better ticket in.

Altho yes, we could put all our tests in the nimble file on the other end.

timotheecour · 2018-10-09T21:07:41Z

Altho yes, we could put all our tests in the nimble file on the other end.

well tests should be in a nim file, but nimble test should call them; at least that's what's standard in nim packages.
Following the most common convention makes things more discoverable and interoperable IMO, especially for tooling.

Here's a concrete example where this matters:

[proposal] run whitelist of popular nimble packages as a part of non-blocking CI suite, to catch regressions early nim-lang/Nim#8638 would enable to run a whitelist of nimble packages; it requires having tests run in a standard way (via nimble test)

sinkingsugar · 2018-10-17T03:06:26Z

We added some tests in nimble file, nimble test works now. Coupled with other tasks too.

timotheecour · 2018-10-17T03:26:36Z

@sinkingsugar after I updated to try the new nimble testbut it fails.
I also tried re-building ATEN using https://github.com/fragcolor-xyz/nimtorch#manual-way-without-requiring-conda (using git reset --hard # from torch/commit.txt , ie 93eea2c58cfe79d43ad514cef471654e0e977b3d) but still fails.

this works:

ATEN=$nim_D/pytorch/build/output/ nim cpp -r -f -o:/tmp/z01 torch.nim

this doesnt:

ATEN=$nim_D/pytorch/build/output/ nimble test

Skipping method with invalid argument/s: to arguments: [{"dynamic_type":"Tensor","is_nullable":false,"name":"self","type":"const Tensor &"}, {"dynamic_type":"Device","is_nullable":false,"name":"device","type":"Device"}, {"dynamic_type":"ScalarType","is_nullable":false,"name":"dtype","type":"ScalarType"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"non_blocking","type":"bool"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"copy","type":"bool"}]
Skipping method with invalid argument/s: to arguments: [{"dynamic_type":"Tensor","is_nullable":false,"name":"self","type":"const Tensor &"}, {"dynamic_type":"Device","is_nullable":false,"name":"device","type":"Device"}, {"dynamic_type":"ScalarType","is_nullable":false,"name":"dtype","type":"ScalarType"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"non_blocking","type":"bool"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"copy","type":"bool"}]
Skipping method with invalid argument/s: to arguments: [{"dynamic_type":"Tensor","is_nullable":false,"name":"self","type":"const Tensor &"}, {"dynamic_type":"Device","is_nullable":false,"name":"device","type":"Device"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"non_blocking","type":"bool"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"copy","type":"bool"}]
Skipping method with invalid argument/s: to arguments: [{"dynamic_type":"Tensor","is_nullable":false,"name":"self","type":"const Tensor &"}, {"dynamic_type":"Device","is_nullable":false,"name":"device","type":"Device"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"non_blocking","type":"bool"}, {"default":false,"dynamic_type":"bool","is_nullable":false,"name":"copy","type":"bool"}]
/Users/timothee/git_clone/nim/nimtorch/torch/generator.nim(490) generator
/Users/timothee/git_clone/nim/Nim/lib/system.nim(3899) failedAssertImpl
/Users/timothee/git_clone/nim/Nim/lib/system.nim(3892) raiseAssert
/Users/timothee/git_clone/nim/Nim/lib/system.nim(2939) sysFatal
Error: unhandled exception: /Users/timothee/git_clone/nim/nimtorch/torch/generator.nim(490, 11) `exitCode == 0` Failed to convert derivatives.yaml to JSON, failed with output: Traceback (most recent call last):
  File "<string>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: '/Users/timothee/git_clone//nim//pytorch/build/output//share/derivatives.yaml'
 [AssertionError]

indeed:

ls $nim_D/pytorch/build/output/share
total 0
drwxr-xr-x 3 timothee 96 Oct 16 20:17 ATen/
drwxr-xr-x 4 timothee 128 Oct 16 20:17 cmake/

sinkingsugar · 2018-10-17T04:00:39Z

You need to copy some files back in the output folder in order to generate derivatives and declarations.

like:

RUN cd && \
cd pytorch && \
mkdir built && \
cd built && \
source activate deps && \
cmake -DCMAKE_INCLUDE_PATH=$CONDA_PREFIX/include -DCMAKE_LIBRARY_PATH=$CONDA_PREFIX/lib -DCMAKE_BUILD_TYPE=Release -DBUILD_ATEN_ONLY=ON -DCMAKE_INSTALL_PREFIX=`pwd`/output ../ && \
make -j10 && \
make install -j10 && \
make clean && \
cp ../tools/autograd/derivatives.yaml `pwd`/output/share/

I will add it to the readme soonish

timotheecour · 2018-10-17T04:04:00Z

ah ok, cp ../tools/autograd/derivatives.yaml pwd/output/share/; thanks!
nit: btw build is more common than built (that's what i had put in readme.md manual build instructions)

sinkingsugar · 2018-10-17T04:11:33Z

Do you think so? I'm actually curious about it..
I put lot of efforts for conda and workflow, specially for people coming from python and without any knowledge of nim.
Also I don't like nimble as package manager either.. often keeps renamed modules alive.. and other weird stuff...

timotheecour · 2018-10-17T04:18:50Z

ok manually adding cp ../tools/autograd/derivatives.yaml pwd/output/share/ made it work; just a bit verbose but ok; looks like torch.nimble is quite exhaustive...

Also I don't like nimble as package manager either.. often keeps renamed modules alive.. and other weird stuff...

I actually quite like it :) not that it can't be improved, but compared to D's dub, this was a pleasant upgrade. Are there specific bugs in https://github.com/nim-lang/nimble/issues/ you're referring to? if they're not there we should definitely add a bug report so it doesn't get lost

I think torch.nimble could be refactored as follows (untested):

let files = ["torch/generator.nim", ..., "tests/test_xor.nim"]
task test_cuda, "Run cuda gpu tests":
  for file in files:
    exec "nim cpp -r -o:test -d:cuda " & file
    exec """nim cpp -d:release -r -o:test -d:cuda torch"""

all files have nim ext in that file except torch => use torch.nim ?
exec """nim cpp -r -o:test torch/generator.nim""" is repeated in several test suites identically; seems like a bug?

sinkingsugar · 2018-10-17T06:15:00Z

Nimble has a major flaw, which is it applies all the packages it has on every project you have by default. That's my major concern that easy can create a mess if not considered.

Do you mind we open a issue about those points above? no time right now but they make sense :)

timotheecour · 2018-10-17T06:36:13Z

Do you mind we open a issue about those points above? no time right now but they make sense :)

=> #11

Nimble has a major flaw, which is it applies all the packages it has on every project you have by default. That's my major concern that easy can create a mess if not considered.

I'd like to understand what you're referring to here; when you have time, do you mind providing more details in particular what kind of mess it can create, and what you'd want it to do instead ? if not a minimal test case at least enough info :-)

the assumption is every package has a unique name, so there should be no name clash;
if nimble is aware of packages foo1 foo2 foo3 foo4, and package foo4 depends only on foo1 foo2, when building foo4 it shouln't have to build foo3; but maybe I'm misunderstanding

sinkingsugar · 2018-10-17T06:55:05Z

Thank you!

I will give you an example exactly with nimtorch:

Have nimtorch installed as nimble package
Also work on the repository
Did not run nimble develop
Rename a file in the repository
Forget to update the module import myrenamedfile into import newlocation/myrenamedfile
Build will succeed yet will use import myrenamedfile from nimble this time..

timotheecour · 2018-10-17T17:41:00Z

=> moved to #12 to keep each issue separate

timotheecour changed the title ~~nim cpp -r torch.nim => nimble test ? (more standard)~~ [minor] nim cpp -r torch.nim => nimble test ? (more standard) Oct 8, 2018

sinkingsugar added the enhancement New feature or request label Oct 8, 2018

sinkingsugar closed this as completed Oct 17, 2018

timotheecour mentioned this issue Oct 17, 2018

[minor] torch.nimble improvements #11

Open

timotheecour mentioned this issue Oct 17, 2018

[TODO] [offtopic] discussion regarding nimble limitation (from comments in #6) #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[minor] `nim cpp -r torch.nim` => `nimble test` ? (more standard) #6

[minor] `nim cpp -r torch.nim` => `nimble test` ? (more standard) #6

timotheecour commented Oct 8, 2018

sinkingsugar commented Oct 9, 2018

timotheecour commented Oct 9, 2018

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018 •

edited

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018 •

edited

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018 •

edited

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018

[minor] nim cpp -r torch.nim => nimble test ? (more standard) #6

[minor] nim cpp -r torch.nim => nimble test ? (more standard) #6

Comments

timotheecour commented Oct 8, 2018

sinkingsugar commented Oct 9, 2018

timotheecour commented Oct 9, 2018

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018 • edited

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018 • edited

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018 • edited

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018

sinkingsugar commented Oct 17, 2018

timotheecour commented Oct 17, 2018

[minor] `nim cpp -r torch.nim` => `nimble test` ? (more standard) #6

[minor] `nim cpp -r torch.nim` => `nimble test` ? (more standard) #6

timotheecour commented Oct 17, 2018 •

edited

timotheecour commented Oct 17, 2018 •

edited

timotheecour commented Oct 17, 2018 •

edited