Update the guideline of RFC tracking issues #13
Conversation
Update the guideline of RFC tracking issues
|
Hi @comaniac, Thanks for suggesting this, it's been on my mind 😸 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? |
|
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 |
|
LGTM thanks @comaniac |
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. |
|
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). |
|
@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. |
|
@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:
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. |
|
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. |
Thanks for updating this, I think this is much better as it gives responsibility to the committer to manage whether the issue is required or not. |
Based on the two RFCs I've reviewed, there are two points related to the RFC tracking issue that could be improved IMHO.