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

[C++][API] Python-style With, Consistent RAII #3231

Merged
merged 1 commit into from
May 24, 2019
Merged

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented May 23, 2019

We have quite a few RAII style scope context throughout the code base, and their naming conventions are not very consistent. This PR aims to provide a unified API for the RAII scoping similar to python's with syntax.

Any context class needs to implement set of standard functions to be implemented by a context class, which implement two functions(EnterWithScope, ExitWithScope) and then we can create an RAII class With.

{
   With<Target> target_scope(target);;
   // target in effect
}
{ 
   With<ConstraintContext> constraint(&analyzer, x % 3 == 0);
   // constraint in effect.
}
  • Refactored Target, BuildConfig, ConstraintContext to use the new style.
  • Renamed BuildConfig::create -> BuildConfig::Create to be consistent with the style
  • Use Target::Current() to indicate the current thread local scope context.

@tqchen
Copy link
Member Author

tqchen commented May 23, 2019

@tqchen tqchen changed the title [REFACTOR] Consistent RAII with API. [C++][API] Consistent RAII with API. May 23, 2019
@tqchen tqchen changed the title [C++][API] Consistent RAII with API. [C++][API] Python-style With, Consistent RAII May 23, 2019
@alex-weaver
Copy link
Contributor

While I agree with making the RAII interfaces more consistent, I'm not convinced that we should be moving the C++ away from idiomatic C++ to cater to python idioms. To my mind, the C++ API should follow C++ conventions (ie. constructor/destructor pairs for RAII enter/exit) and if the python modules require helper functions or wrapper functions, then they should be a layer on top of the base C++ API, so that non-python clients do not need to use them. The motivations for this make sense, but to me adding the With abstraction is turning the implementation inside-out.

I do agree with making RAII more consistent, but my suggestion would be that we follow C++ idioms and establish standard patterns for interfacing with python

@tqchen
Copy link
Member Author

tqchen commented May 23, 2019

Thanks, @alex-weaver . The purpose of this PR is not to move away from C++ idioms, but provide a unified way to construct RAII scopes. Specifically, With<ObjectType> API can be viewed as a C++ API that follows RAII idiom. And provides a standard naming and definition of such an object. The name of the object itself is inspired by the with-syntax of python.

It is similar to lock_guard's relation to different types of mutex, which is adopted by STL. The main reason for such a change is to standardize the RAII objects which are already abundant.

Hope this clarifies a bit, and I would also like to hear about possible alternative examples if you have anything particular in mind.

@MarisaKirisame
Copy link
Contributor

This approach has a problem: the Enter/Exit might be called multiple time from outside, and it is possible that they arent called in the correct order.
I suggest that this code should be in CRTP style, with the With class defining Enter/Exit, which check the type state (uninited/inited/exited) is correct and manipulate it. The individual calss should then defined EnterAux/ExitAux.

@tqchen
Copy link
Member Author

tqchen commented May 23, 2019

The CRTP style is a bit more intrusive. For now, the recommendation is always to keep Enter/Exit function private and declare a friend class to With, in which case Enter/Exit can only be called by With

@alex-weaver
Copy link
Contributor

@tqchen I guess I'm not seeing what the With abstraction buys over directly implementing a context class with constructor and destructor for things that can live in contexts. Then there is no need to tie the context implementation to the object class; to me these are separate concerns.

So for example, instead of this:

{
   With<Target> target_scope(target);;
   // target in effect
}

where Target is responsible for both representing a target and a stack of contexts, why not simply:

{
    TargetScope target_scope(target);
}

Then the responsibility of maintaining scopes is cleanly separated from the objects themselves, and the standardization can come from adopting naming conventions and semantics for these scope objects.

@tqchen
Copy link
Member Author

tqchen commented May 23, 2019

I see. I think it has things to do with how the scope semantics is implemented(in the scope object or the context object themselves).

From user’s perspective modulo naming(With vs TargetScope).

So back to the implementation point of view. I do not have strong opinion in either way. We can even achieve the same API if we adopt the With convention and implement the logic in that class(via template specialization). The fact that we try to hide the enter/exit function under private is exactly because we don’t want the user to bother such details. This being said, having a separate enter/exit function indeed makes frontend wrapping easier. Given similar style(lock/unlock and lock guard) already presents in the str, I feel we can still use this implementation, but I do not feel strongly it should be the case as long as we present the same api.

So there are two point being discussed:

  • The user scoping API convention,
    which we hope to use With
  • How can this API be implemented(in scope or in the object)

@tqchen
Copy link
Member Author

tqchen commented May 24, 2019

@alex-weaver given that there are followup PR which might depend on this one. I propose to merge it tomorrow. The idea is that we will introduce With as the RAII convention, but do not want to force an opinionated view on how the RAII is implemented(in the XYZ::Enter/Exit or in With itself) as long as the API remains the same.

@alex-weaver
Copy link
Contributor

@tqchen Yes, it does make sense to separate the API choice from the implementation.
As far as the API goes, it wasn't clear to me why it is useful to standardize on With. I guess one argument that could be made is an ergonomic one - as a user of the API you automatically know the type signature of the scope object. The parallel alternative would be a consistent naming scheme for scope objects.

I agree that the API can hide different implementations, but my preference would be for the default to be separated implementation.

It's not a strong preference either way, I just have a slight preference for what would make the concerns separate by default, and be "least surprising"; but I don't mind this merging as-is.

@tqchen tqchen merged commit 415a270 into apache:master May 24, 2019
@tqchen
Copy link
Member Author

tqchen commented May 24, 2019

Thanks @alex-weaver @MarisaKirisame for helpful discussions

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
@tqchen tqchen deleted the with branch October 31, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants