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

[Lang] Support formatted string with args in assert #1806

Merged
merged 4 commits into from
Aug 30, 2020

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Aug 29, 2020

This allows us to write

assert cond(), 'foo=%d bar=%f' % (foo, bar)

I think we can apply the same strategy to print, but let me focus on assert in this PR.

Related issue = #1434

[Click here for the format server]


# taichi_lang_core.Expr (defined in C++)
import taichi as ti
taichi_lang_core.create_assert_stmt(
ti.Expr(cond).ptr, msg, [ti.Expr(x).ptr for x in extra_args])
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if extra_args is Vector? Let's make use of Vector.__ti_repr__ like currently ti_print does.

Copy link
Member Author

@k-ye k-ye Aug 30, 2020

Choose a reason for hiding this comment

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

I see. Unfortunately, assert format specifier doesn't support such a fancy data structure yet, see

if (dtype == 'd') {
error_message_formatted += fmt::format(
"{}", taichi_union_cast_with_different_sizes<int32>(argument));
} else if (dtype == 'f') {
error_message_formatted += fmt::format(
"{}",
taichi_union_cast_with_different_sizes<float32>(argument));
} else {
TI_ERROR("Data type identifier %{} is not supported", dtype);
}

It only has %d or %f..

I think this comes from the fact that, right now print and assert handle args differently. assert only handles string with format specifiers, which limits what kind of runtime data we can put into the message.

@archibate
Copy link
Collaborator

archibate commented Aug 29, 2020

I'd suggest us to translate:

assert cond, "foo=%f" % foo

into:

if cond:
  pass
else:
  print("foo=%f" % foo)
  ti.core.insert_assert_failure_stmt()

for API consistence and maintainability. So that we could deprecate AssertStmt, and use AssertFailureStmt plus IfStmt instead.

@codecov
Copy link

codecov bot commented Aug 29, 2020

Codecov Report

Merging #1806 into master will increase coverage by 0.15%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1806      +/-   ##
==========================================
+ Coverage   43.51%   43.67%   +0.15%     
==========================================
  Files          44       44              
  Lines        6023     6045      +22     
  Branches     1078     1082       +4     
==========================================
+ Hits         2621     2640      +19     
- Misses       3248     3251       +3     
  Partials      154      154              
Impacted Files Coverage Δ
python/taichi/lang/impl.py 67.31% <66.66%> (-0.01%) ⬇️
python/taichi/lang/transformer.py 81.88% <85.71%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3f1a5d...e8794ba. Read the comment docs.

Copy link
Collaborator

@archibate archibate left a comment

Choose a reason for hiding this comment

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

Thanks, this is a very useful feature. But as yuanming always reminds me, we should always discuss about the design, reach an agreement, before start to writing code. My opinion is to translate assert into if + print + ti.assert_failure(). What do you think?

@yuanming-hu
Copy link
Member

My opinion is to translate assert into if + print + ti.assert_failure(). What do you think?

Thanks for proposing. However, we actually want AssertStmt be a single and "atomic" statement instead of multiple statements. We need to write the assertion failure message to the result buffer, and using print will not do the same job.

@k-ye
Copy link
Member Author

k-ye commented Aug 30, 2020

My opinion is to translate assert into if + print + ti.assert_failure(). What do you think?

Yeah, this seems cleaner. However, note that this is likely a larger change than expected:

  1. print and assert handle args quite differently. Specifically, print doesn't support format specifiers yet, which would require the C++ side change.
  2. We may also want to support printing to different std streams, so that assertion goes to stderr? (BTW, right now assertion messages are printed with TI_ERROR)

I'd propose to support this first just because users can start using it, then unify how print/assert handles their string arguments. print may be more complicated, because we need to support things like print('foo=%d bar=%f' % (foo, bar), ' hello', 123).


Forget what i said....

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.

Thank you so much! LGTM. This can be merged after committing the suggestion in test_assert_minimal.

tests/python/test_assert.py Show resolved Hide resolved
@yuanming-hu
Copy link
Member

I'd propose to support this first just because users can start using it, then unify how print/assert handles their string arguments. print may be more complicated, because we need to support things like print('foo=%d bar=%f' % (foo, bar), ' hello', 123).

Maybe we should also consider supporting f-strings in the future :-)

@k-ye
Copy link
Member Author

k-ye commented Aug 30, 2020

Maybe we should also consider supporting f-strings in the future :-)

Ah yes, that sounds like a much bigger feature, though..

@k-ye k-ye requested a review from archibate August 30, 2020 02:02
@k-ye k-ye merged commit 58363bb into taichi-dev:master Aug 30, 2020
@k-ye k-ye deleted the py-assert branch August 30, 2020 09:00
@yuanming-hu yuanming-hu mentioned this pull request Sep 1, 2020
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.

3 participants