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

Use argparse in taichi/main.py to implement #600 #601

Merged
merged 5 commits into from
Mar 18, 2020

Conversation

archibate
Copy link
Collaborator

Related issue id = #600

@archibate
Copy link
Collaborator Author

archibate commented Mar 15, 2020

Note that no more test = test_python + test_cpp, we use test = test_python now for simplicity and since we usually want test_python. So I use ti test && ti test_cpp in CI ymls.
Will be fixed with further --cpp arguments.

@archibate
Copy link
Collaborator Author

archibate commented Mar 15, 2020

plan A, lower into TI_ARCH:
ti test --arch=opengl numpy
ti debug --arch=opengl a.py

plan B, hack in all_archs:
ti test --arch=opengl numpy
ti test --arch=opengl,metal numpy

AFK take eye rest.

@archibate
Copy link
Collaborator Author

archibate commented Mar 15, 2020

Guess what? Appveyor:

================================== FAILURES ===================================
_______________________ tests/python/test_ad_atomic.py ________________________
[gw1] win32 -- Python 3.6.8 C:\Python36-x64\python.exe
worker 'gw1' crashed while running 'tests/python/test_ad_atomic.py::test_ad_reduce'
=========================== short test summary info ===========================
FAILED tests/python/test_ad_atomic.py::test_ad_reduce
================== 1 failed, 279 passed in 985.21s (0:16:25) ==================

Confusing crash again.. We may want to have an issue for this. Also notable test time 16m25s.. We used to have this time to build&test all done..

@yuanming-hu
Copy link
Member

Note that no more test = test_python + test_cpp, we use test = test_python now for simplicity and since we usually want test_python. So I use ti test && ti test_cpp in CI ymls.
Will be fixed with further --cpp arguments.

test_cpp takes < 0.1 second, so I think we can actually enable it by default.

@yuanming-hu
Copy link
Member

Guess what? Appveyor:

================================== FAILURES ===================================
_______________________ tests/python/test_ad_atomic.py ________________________
[gw1] win32 -- Python 3.6.8 C:\Python36-x64\python.exe
worker 'gw1' crashed while running 'tests/python/test_ad_atomic.py::test_ad_reduce'
=========================== short test summary info ===========================
FAILED tests/python/test_ad_atomic.py::test_ad_reduce
================== 1 failed, 279 passed in 985.21s (0:16:25) ==================

Confusing crash again.. We may want to have an issue for this. Also notable test time 16m25s.. We used to have this time to build&test all done..

Yeah for some reason on Windows sometimes it fails randomly. Haven't figured out why.

Also it's true that test time needs improvement.

@yuanming-hu
Copy link
Member

plan A, lower into TI_ARCH:
ti test --arch=opengl numpy
ti debug --arch=opengl a.py

plan B, hack in all_archs:
ti test --arch=opengl numpy
ti test --arch=opengl,metal numpy

AFK take eye rest.

Plan B sounds better :-)

@archibate
Copy link
Collaborator Author

Already used plan B now :)

@yuanming-hu
Copy link
Member

Is this one ready for review?

@archibate
Copy link
Collaborator Author

Is this one ready for review?

Yes!

@archibate
Copy link
Collaborator Author

I've thought about a good idea eval(f'do_taichi_{args.action}()') to make something same as dict, but replaces if-else-if.

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Looks great basically. The only thing to change: make test cover both the old test_python and test_cpp?

.travis.yml Outdated
@@ -56,7 +56,7 @@ script:
- export TAICHI_REPO_DIR=$TRAVIS_BUILD_DIR
- export PYTHONPATH=$TAICHI_REPO_DIR/python
- export PATH=$TAICHI_REPO_DIR/bin:$PATH
- ti test_verbose && cd python && $PYTHON build.py try_upload
- ti test -v && ti test_cpp -v && cd python && $PYTHON build.py try_upload
Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we merge test_cpp into test: #601 (comment)

python/taichi/lang/__init__.py Show resolved Hide resolved
python/taichi/main.py Outdated Show resolved Hide resolved
@yuanming-hu
Copy link
Member

I've thought about a good idea eval(f'do_taichi_{args.action}()') to make something same as dict, but replaces if-else-if.

I think this is a great idea! We should do it in the future - please feel free to open up an issue to track it. I'm working on the code formatting issues right now.

@archibate
Copy link
Collaborator Author

Btw, isn't it better use return -1 instead of exit(-1)? Might call taichi.main and return codes.

@yuanming-hu
Copy link
Member

Btw, isn't it better use return -1 instead of exit(-1)? Might call taichi.main and return codes.

Good point!

Comment on lines 133 to 136
ret = test_python(test_files=args.files, verbose=args.verbose)
if ret: exit(-1)
ret = test_cpp(test_files=args.files)
return ret
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here comes the problem: How can we distinct cpp test_files and python test_files? Still need test_cpp cmd?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... Maybe use test -py [files] and test -cpp [files...] to specify the files? It will be much cleaner if a single root command test can be used.

@archibate
Copy link
Collaborator Author

archibate commented Mar 18, 2020

I've thought about a good idea eval(f'do_taichi_{args.action}()') to make something same as dict, but replaces if-else-if.

I think this is a great idea! We should do it in the future - please feel free to open up an issue to track it. I'm working on the code formatting issues right now.

Something even better is to use class TaichiMain, contains do_{args.action} (access using getattr acts like visit_Return), and self.parser (currently so many dirty global..), enable us to have multiple taichi instances, no more re-ti.init for all_archs serialized test.

@yuanming-hu
Copy link
Member

yuanming-hu commented Mar 18, 2020

I've thought about a good idea eval(f'do_taichi_{args.action}()') to make something same as dict, but replaces if-else-if.

I think this is a great idea! We should do it in the future - please feel free to open up an issue to track it. I'm working on the code formatting issues right now.

Something even better is to use class TaichiMain, contains do_{args.action} (access using getattr acts like visit_Return), and self.parser (currently so many dirty global..), enable us to have multiple taichi instances, no more re-ti.init for all_archs serialized test.

Sounds like a good plan for a future PR. I would merge this in as soon as possible to prevent the changesets of this PR becoming too big...

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

Looks great now!! Thank you so much.

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.

2 participants