tqchen commented on PR #14094: URL: https://github.com/apache/tvm/pull/14094#issuecomment-1441830020
Thanks, everyone, for chiming in so far. We have spent quite a bit of time over this. I think we are getting closer to more clarity so I would like to provide a digest of my verdict so far. ## Importance of Scoping Here scoping refers to clearly naming and differentiating the overall modules with other means. For example, when there is already a module (tvm rust api) that interfaces with the users. It would be appropriate to find ways to structure it clearly in the codebase so that it differentiates with existing modules. This **does not mean** that the proposed new module should not overlap with the features provided by existing modules. It does not limit the scope of what the API can do but asks for clarification of API and implementation such that they signal the difference to the user. It only means that the proposed API and related code sit in a way that signals the difference and structure. Nor does it specify the way the API is named. For example, naming it nostd_rust_interface would also be accurate, not restricting the setting to embedded but also signaling that it is a different module from the current rust API. Naming it tvm_awesome_xyz_rust.cc also works as long as awesome_xyz conveys the intent of the code. There are many approaches to scoping, including, but not limited to: - naming of the respective files - subfolder structures - namespace These practices are also common practices in software development. ## Scoping is a Necessary Precondition for Agility We are a project that comes with a lot of different modules and developer communities. Each one of us may care about a different subarea. In order for the modules to be sufficiently developed concurrently. Clarity of scoping (again, not in terms of what they can do, but simply in terms of clear organization, naming, and namespacing along with existing architecture) is a necessary condition for us to empower each other so as to avoid one module stepping into another one. Usually, proper scoping needs to be done once – e.g. placing things in a proper subfolder(and/or) namespace would allow agile development of any APIs under that namespace without much intervention from the broader community. This is because the scope of impact is also isolated in such a way. After proper scoping, there will be great flexibility. Naming the API as a namespace_x::awesome_xyz is fine as long as the community members of respective areas agree. One would have great flexibility as long as the namespace differentiates it from existing namespace, and additional folder structure that helps to signal clear organizations. That does not mean we cannot bring changes to the root namespace, they do mean that would certainly need more deliberations in consideration along with existing APIs and an evaluation from an overall architecture standpoint. ## Scoping is the Standard Practice in Almost All Modules in TVM Scoping is a standard practice, and necessary condition for software to scale and is the standard practice in the TVM project. We can easily name a few: meta_schedule (so name differentiates from autotvm), uma, crt. This is a standard practice that most of us follow in the project. They do not diminish the value, or what the module can do, they just help to bring a clear structure. I worked on these properly scoped modules daily. When reviewing and accepting RFCs, we always keep assuming that the code will be properly scoped, or feedback being taken to resolve that goal. ## Verdict of This Particular Case In this particular case, the items of interest includes three things A: the overall architecture, which already includes a rust API that interacts with the standard tvm runtime. y: proposed interface_rust, which has an scoping issue mainly to have possible confusions with the existing rust API. x: interface_c , the “old architecture” that was referred to in this case, which unfortunately was not properly scoped, as it can cause confusion with the c_runtime_api. Since interface_c is not compatible with all variants of runtimes and is reasonably strongly coupled with crt(for a good reason). Naming it as something like interface_c_for_crt, interface_c_tiny would have been better. I have not reviewed x’s code change unfortunately, and the respective RFCs did not discuss scoping. Still the priority in terms of architecture should be prioritizing y to be consistent with A, even if it may come at the cost of slight inconsistency with x. The suggestion has nothing to do with whether or not to restrict the usage to embedded only. But instead to clearly signal the difference in the space. For example, nostd_rust would be another accurate description that also works, as it does not depend on the larger standard runtime (including object system) that TVM provides (for a good reason). Any other proper namespacing or folder structure also works. There is also no inconsistency with the recent rationales for agility and empowerment. As all of the other threads are pre-conditioned on proper scoping of such related modules. It is also not an issue that will arise in the future, as the rust API already exists in the codebase now. Putting the changes in a reasonable subfolder, namespace, or at least proper naming would enable us to iterate the proposed API in an flexible way. In this way we can empower not only this PR, and other community developers (who also follow the same scoping principle). Because the proposal has a need for scoping, it becomes a case of overall TVM architecture. I have volunteered my energy over years to help coordinate the overall architecture and hopefully the overall input gives more clarity here. I also explained the rationale. Hopefully the recommendations are clear and they are made on the basis of the state of scoping and the overall architecture. If there is a goal to continue code review and iterate without addressing the scoping, then a branch could be a good starting point. Other ways to achieve the same goal also helps here. Again I am not being fixated on only one way to name the file, but instead offering suggestions to help the scoping of the module. I have no doubt that the module could be useful to intended users and developers, just that for the module to co-exist with other modules in the project some minimum requirement of scoping is needed to clear it from overall architecture pov. I would be more than happy to review future proposals. To give extra clarity, I marked the PR as need-scoping. Please copy me on future proposals related to the module as I would like to help on the scoping part. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
