[apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
Based on the two RFCs I've reviewed, there are two points related to the RFC tracking issue could be improved IMHO. 1. We currently state that the tracking issue should be opened after the RFC PR is merged. However, it means the author needs to file another PR to update the issue link, which seems not necessary to me. As a result, I refined the description, saying that the issue should be opened when the review is nearly done. The reviewer should also remind authors to open a tracking issue and update the link before merging the RFC PR. 2. To better tracking the RFC issues, I created a new label "rfc-tracking". It would be better to add this label to all RFC tracking issues associated with the RFCs proposed here. You can view, comment on, or merge this pull request online at: https://github.com/apache/tvm-rfcs/pull/13 -- Commit Summary -- * Update the guideline of RFC tracking issues -- File Changes -- M README.md (5) -- Patch Links -- https://github.com/apache/tvm-rfcs/pull/13.patch https://github.com/apache/tvm-rfcs/pull/13.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
cc @tqchen @hogepodge @areusch @jroesch -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887681011
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
Hi @comaniac, Thanks for suggesting this, it's been on my mind :smile_cat: I'd suggest that "nearly done" is ambiguous? As a less ambiguous alternative I'd propose always opening a tracking issue (if the RFC is big enough to require it) when you raise an RFC and if it ultimately gets rejected we just close the issue? This also allows code to evolve alongside the RFC. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887686396
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
> I'd suggest that "nearly done" is ambiguous? As a less ambiguous alternative > I'd propose always opening a tracking issue (if the RFC is big enough to > require it) when you raise an RFC and if it ultimately gets rejected we just > close the issue? This also allows code to evolve alongside the RFC. I was thinking about that too, but I'm afraid that it might be confusing to see lots of RFC tracking issues (even they are closed). After all, deleting an issue is not allowed in Github, so it should be better to do our best to have only the tracking issues of accepted RFCs. Meanwhile, I do agree that "nearly done" is too vague. I might rephrase it to "when the RFC is about to be accepted, a committer should remind authors to open a tracking issue and update the link before merging". How does that sound? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887690611
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
cc @apache/tvm-committers for awareness. Given this change seems to be non-controversial, we can adopt lazy consensus and merge after 3 days if no objection comes up -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887690812
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
LGTM thanks @comaniac -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887696449
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
> > I'd suggest that "nearly done" is ambiguous? As a less ambiguous > > alternative I'd propose always opening a tracking issue (if the RFC is big > > enough to require it) when you raise an RFC and if it ultimately gets > > rejected we just close the issue? This also allows code to evolve alongside > > the RFC. > > I was thinking about that too, but I'm afraid that it might be confusing to > see lots of RFC tracking issues (even they are closed). After all, deleting > an issue is not allowed in Github, so it should be better to do our best to > have only the tracking issues of accepted RFCs. > Deleting an issue is not something we should do - keeping a record of why something was rejected is useful on a later date to know why but perhaps the issue of too many issues with the label is solved by a query for open rfc-tracking issues ? Ramana -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887696764
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
> Deleting an issue is not something we should do - keeping a record of why > something was rejected is useful on a later date to know why but perhaps the > issue of too many issues with the label is solved by a query for open > rfc-tracking issues ? > > Ramana I think the record would always be kept here. IIUC, tracking issues in the main repo are only used to track the RFC implementation progress, which is not necessary for rejected RFCs. IMHO this can also save some times for authors: If an RFC is rejected, then authors don't even need to waste time proposing an implementation plan. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887701334
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
i don't mind if we have a bunch of closed tracking issues. we can categorize them. i agree with @u99127 that maintaining a record is important, and i also think it makes sense that the first PR to land on a tracking issue would be its RFC. i think committers should be able to notify authors at the time that it makes sense to create an issue, if authors don't do it pre-emptively. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887736594
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
> i don't mind if we have a bunch of closed tracking issues. we can categorize > them. i agree with @u99127 that maintaining a record is important, and i also > think it makes sense that the first PR to land on a tracking issue would be > its RFC. i think committers should be able to notify authors at the time that > it makes sense to create an issue, if authors don't do it pre-emptively. I personally don't like a bunch of useless issues. Most issues in main TVM repo don't categorize well, so it might be confusing if we attempt to add more. Meanwhile, I don't think we will lose any record if tracking issues aren't opened for rejected RFCs, as all important records and discussions should be documented in the RFC PR. In contrast, tracking issue is just a "tracking" issue. If it has important information we don't have in the RFC, then it should be something wrong IMHO. On the other hand, of course we cannot prevent authors from creating an issue at the time of filing the RFC PR if they prefer. We just don't explicitly enforce them to do so. I think that's why the current guideline suggests creating a tracking issue after merging RFC PR, and I do agree with this policy. With the updated guideline, we just ask committers to be in charge of making sure the issue is created before merging the RFC PR and help update the issue (add the RFC ID after merged and the label). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887750459
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
@comaniac the RFC in my mind is mainly a design-level description of some aspect of TVM. Like e.g. PEP, they are meant to be consumed by people less familiar with the TVM codebase in order to gain familiarity. the tracking issue, on the other hand, documents the method and progress by which the RFC changes were applied to the TVM codebase, as well as serves as a convenient place to collect limitations (e.g. people can report major, clearly-related problems, or link downstream issues, and readers can track the efforts of resolving those things either on the tracking bug or on the downstream issue). I think that while the RFC should get updated if issues are discovered during implementation that affect the final result, the log of PRs submitted to implement an RFC is too much detail for an RFC. Tracking issues are pretty easy to categorize, as well--they are clearly distinct from any other issue and it's not hard for a reviewer to e.g. label them at the time of merging the RFC. So I'm not sure I see how they will be problematic to the overall organization. I agree if an RFC is rejected, we don't lose anything by not opening a tracking issue. I similarly don't think it's a big deal to have a bunch of closed tracking issues for RFCs in the event that the issue was opened before the RFC was rejected. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887764234
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
> @comaniac the RFC in my mind is mainly a design-level description of some > aspect of TVM. Like e.g. PEP, they are meant to be consumed by people less > familiar with the TVM codebase in order to gain familiarity. > > the tracking issue, on the other hand, documents the method and progress by > which the RFC changes were applied to the TVM codebase, as well as serves as > a convenient place to collect limitations (e.g. people can report major, > clearly-related problems, or link downstream issues, and readers can track > the efforts of resolving those things either on the tracking bug or on the > downstream issue). I think that while the RFC should get updated if issues > are discovered during implementation that affect the final result, the log of > PRs submitted to implement an RFC is too much detail for an RFC. > > Tracking issues are pretty easy to categorize, as well--they are clearly > distinct from any other issue and it's not hard for a reviewer to e.g. label > them at the time of merging the RFC. So I'm not sure I see how they will be > problematic to the overall organization. I agree if an RFC is rejected, we > don't lose anything by not opening a tracking issue. I similarly don't think > it's a big deal to have a bunch of closed tracking issues for RFCs in the > event that the issue was opened before the RFC was rejected. What you mentioned makes sense to me, but it seems not a case at least for the current accepted and reviewed RFCs, and that's why I got this impression: the interface/API/underlying designs are documented in the RFC while the tracking issue only lists the implementation milestones linked to the PRs. If we all agree that the RFC here should retain high-level design without mentioning the interaction between TVM codebase, it would be better to refine the guideline as well as RFC/issue templates. For example, the statement in the Reference-Level explanation section in RFC template has "It is reasonably clear how the feature would be implemented." IMHO, it is inevitable to mention some details in the TVM codebase to make a clear explanation. On the other hand, my experience is that although one may propose a 100% reasonable RFC, the implementation is totally not acceptable. That's why I personally consider it would be better to also discuss the implementation in the RFC. We just need to make a clear claim that this part is highly related to TVM codebase and encourage readers to skip the section if they just want to know the high-level idea. Anyways, it seems a bit off to discuss the scope of RFC and tracking issue in this PR. In summary, what this PR wants to make sure is that tracking issues are opened before merging RFCs. The topic of whether to explicitly request authors to open a tracking issue upon filing an RFC PR (i.e., move the **Tracking Issue** step before **Pull Request** in README) can be continued in another PR. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887787785
Re: [apache/tvm-rfcs] Add Project API RFC (#8)
>> On generate_project method, and considering TVMC, my only comment is that it >> should allow a project creation based also on MLF .tar, instead of only a >> "live" executor. >I agree; let's merge additional logic in project.py to do this as follow-on to >this impl. Sure! I'll submit a follow up patch I've been using to test this patchset with TVMC. My comment was also more for the records for others following the discussion since we've already discussed the details about. >> On build method I just think it should be a way to force a rebuild in the >> same project dir (i.e. even if build dir exists) > I believe that is the functionality now. Did you see something different? Yes, a functionality detail. Currently I'm handling that at TVMC side, like if the `build/` dir exists it means `tvmc micro build` was run before so it informs the user and asks to confirm a re-build using a `--force` flag. In that case, if `--force` is used TVMC will remove the `build/` and let Project API recreate it. But I was just wondering if you would like to change that behavior. I have no preference here. What do you think? >> Also, if possible, please consider the comments I've posted to the draft >> code also (apache/tvm#8380) >Will take a look at these now that PoC is passing OK. I think you answered all my initial comments there. I'll just post a couple more related to that new round, i.e. related to last commit pushed: https://github.com/apache/tvm/commit/de75022daeb889682db80f9e90ab58b3eee898dd I have no more comments regarding the RFC itself - I'm happy with it, so I'll just post some comments about the code. I think it's pretty close to land :) >>Finally, I confirm that the fixes for MAX_ defines are ok (no build error) >>and the per board configs are kicking in correctly. > I reworked the way these are done since posting the PoC--now prj.conf is > generated from microtvm_api_server.py so we can have a single combined > template_project and the prj.conf can be situation-specific. Let me know what > you think. Yeah, I'm not much inclined to use that approach of using a Python code to generate the `prj.conf` because for adding new bits to the config fil will imply changing a Python code. That seems a bit strange to me. I thought of having a `prj.conf` per `project_type` but there is also other things to being handled like the stack configuration based on running the model on QEMU or a physical board, so I'm not sure, maybe we can go ahead with that approach and see it goes overtime? Regarding the `MAX_` defines, even with the new approach generating the `prj.conf` the `crt/crt_config.h` is used and so the `MAX_` defines are used anyways? Or I missed something? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/8#issuecomment-887842848
Re: [apache/tvm-rfcs] Add Project API RFC (#8)
@Mousius please take another look when you can and explicitly approve! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/8#issuecomment-887879286
Re: [apache/tvm-rfcs] [RFC] [Relay] Automatic Mixed Precision Pass (#6)
Thanks for driving this review @comaniac. I'll get to this later in the week. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/6#issuecomment-887889315
Re: [apache/tvm-rfcs] Add Project API RFC (#8)
@tkonolige @guberti @mehrdadh please take another look and explicitly approve if you're good w/ this -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/8#issuecomment-887894876
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
@comaniac i view the core data structures and interfaces as part of the design-level documentation. i think they belong in Reference-level explanation. see for example my RFC #8 --this includes the introduced interface as part of the RFC. details such as where code lives are not considered in the RFC. i don't think an RFC is really very well-written if a reviewer finds the RFC acceptable but the implementation 100% unacceptable beyond a reasonable code-review process. code review should be about aligning on implementation details, and should be guided by the goals set forth in the RFC. another thought I had while considering this proposal: there could be an alternate flow as follows: - proposal thread on Discuss forum - initial sketch of RFC sent as draft PR to tvm-rfcs plus tracking bug - PMC/Committers review RFC sketch and approve/reject - at this point, RFC could either be committed or just assigned an ID number (e.g. commit an empty file to tvm-rfcs to hold the number and associate tracking bug) - implementation proceeds, tracked by bug - to mark the conclusion of RFC implementation, author updates the RFC PR as-built and merges it. the benefits of this arrangement is that there would be a pending code-review where people reviewing the implementation could request additional design-level documentation as complexities are found in implementation. i'm not too sure if i'm attached to this proposal or not yet, but documenting my thought here as it's related. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887914798
Re: [apache/tvm-rfcs] Update the guideline of RFC tracking issues (#13)
I'm not sure I envisioned the tracking issue number/link be explicitly placed into the RFC. The tracking issue referencing back to the RFC PR and and RFC itself seems like it would be sufficient, but I can understand the desire to codify that as part of the RFC. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/apache/tvm-rfcs/pull/13#issuecomment-887986659
