[GLUTEN-10613][VL] Add experimental method to ConfigBuilder and document experimental config#10659
Conversation
|
Run Gluten Clickhouse CI on x86 |
yeah, I think so. |
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
One small suggestion. Thanks.
| val VELOX_BROADCAST_BUILD_RELATION_USE_OFFHEAP = | ||
| buildConf("spark.gluten.velox.offHeapBroadcastBuildRelation.enabled") | ||
| .internal() | ||
| .experimental() |
There was a problem hiding this comment.
If we always assume experimental configs fall under the internal category, I think it would be better to add an assertion check in the experimental API to ensure they have been marked as internal. Otherwise, I'm concerned that developers might forget to mark experimental configs as internal, potentially making experimental configs public unintentionally.
There was a problem hiding this comment.
@philo-he I have also thought add check here during development. But experimental and internal behavior should be orthogonality. And if we add check, we must first call internal and then call experimental which also is confusing.
There was a problem hiding this comment.
Thanks for your reply. My point is we should establish a clear convention to developers and users. It may sound good to make internal and experimental independent.
We can view internal configs as those intended for developers or advanced users only. They can also be experimental—for example, some unstable tracing configs for debugging can be experimental. On the other hand, not all experimental configs are internal. If they can be exposed to end users for early evaluation even though still under experimental, they shouldn't be marked as internal.
Possible rule
- Public/internal: Who should see/use it
- Experimental: How reliable it is (can be marked as either public or internal based on the first item)
Based on this rule, many experimental configs can be marked as public if they can be exposed to end users.
If this makes sense, we need to update the comments for the internal API, where experimental configs are previously viewed as internal always. And we also need to note that some experimental configs may fall under public category and some under internal category. Then, I feel it may be not good to have a dedicated section to document experimental configs, considering possible duplication. Instead, it may be better to just keep two sections in document: public and internal.
Actually, I am not quite sure which convention is clearer to users and developers. It seems always putting experimental in internal category is clearer? Discussion is welcome. Thanks.
There was a problem hiding this comment.
@philo-he @zjuwangg could we move this to a discussion topic for more attention in the community? this is a important update on adding new features, bigger than the purpose of update the documents in this patch.
To me the experimental section would be better to be public otherwise it maybe difficult for people to test it
There was a problem hiding this comment.
Possible rule
Public/internal: Who should see/use it
Experimental: How reliable it is (can be marked as either public or internal based on the first item)
I second this idea.
Then, I feel it may be not good to have a dedicated section to document experimental configs, considering possible duplication
Maybe we also can only list public(but not experimental) config and experimental section.
There was a problem hiding this comment.
Just update internal() comments and follow following rule to mark config and gen doc.
Possible rule
Public/internal: Who should see/use it
Experimental: How reliable it is (can be marked as either public or internal based on the first item)
|
Run Gluten Clickhouse CI on x86 |
philo-he
changed the title
experimental method to ConfigBuilder and add experimental config section to docexperimental method to ConfigBuilder and document experimental config
…ument experimental configs (apache#10659)
* [GLUTEN-10450][VL] Reclassify internal/public configs and remove internal configs from doc (#10603) Remove internal configurations from gluten doc as described (#10450) * [GLUTEN-10613][VL] Add `experimental` method to ConfigBuilder and document experimental configs (#10659) * Update config doc * [VL] Fix Arrow URL typo (#10641) * Fix enhanced failure --------- Co-authored-by: Terry Wang <zjuwangg@foxmail.com> Co-authored-by: Joey <joey.ljy@alibaba-inc.com>
4 participants
What changes are proposed in this pull request?
Add
experimentalmethod to ConfigBuilder and add experimental config to doc as in #10613How was this patch tested?
NA