Yicong-Huang commented on PR #4495: URL: https://github.com/apache/texera/pull/4495#issuecomment-4322640086
> I agree with @aglinxinyuan's concern on the size of this PR. It's not a good practice we want to follow. At the same time, we do want to include this important AI feature in the first release, and we want to do it before an important conference. Check the following public threads: > > * GitHub discussion about "Emphasize AI in the first Apache release" [Emphasize AI in the first Apache release #4404](https://github.com/apache/texera/discussions/4404) > * Email on the dev list about the timing: https://lists.apache.org/thread/tygpzb9fbzkrrn1bkr98dx1ydhohpcch > * Email on the dev list about the testing of the release: https://lists.apache.org/thread/hjngl8zttvq9fjtyckl72mqmbtpqo5cr > > @Yicong-Huang , @zuozhiw, and All: Is there any way to solve this problem? Ahh this is a tough question. It is definitely not a good idea for having such a large PR. A PR with this size is hardly reviewable. It always imposes a bigger risk of shipping with bugs, break other services/features, or even introduce security concerns. That said, given our timeline of pushing release, this feature is needed and I think we need to assess whether we want to take the risk or not. My take is that this is a new feature, and large file and LoC changes are somewhat expected for work of this nature. We can still choose to accept it with a relatively shallow pre-merge review, but we should be explicit that doing so means taking on some review risk. Some of the methods to reduce the risk level: 1. can we label this feature as "experimental", visibly on the source code (e.g.., package name, decorator) and UI (on the panel itself)? This can reduce the user expectation. 2. can we by default turn the AI feature off so it won't take effect, and won't affect other components/services? Users need to explicitly change a config to turn it on, so that they understand the risk (kind of like an opt in). 3. meanwhile, we can try our best to review this codebase, and do more post merge reviews. WDYT? -- 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]
