Skip to content

document internal/controller#205

Merged
gflarity merged 6 commits into
ai-dynamo:mainfrom
gflarity:gflarity/document_internal_controller
Oct 22, 2025
Merged

document internal/controller#205
gflarity merged 6 commits into
ai-dynamo:mainfrom
gflarity:gflarity/document_internal_controller

Conversation

@gflarity

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This PR improves to documentation for operator/internal/controller tree. See prompt below.

Special notes for your reviewer:

Documentation prompt:

Improve the inline documentation for for all golang files under this directory/package. Here are the rules to follow:

  • Use concise idiomatic go documentation principles.
  • Ensure correctness.
  • Add some minimal inline documentation above complex code blocks when to explain the intent of the code.
  • Don't document code that is clearly self documenting.
  • Blocks of code should only have a comment describing them if it is practical and valuable.
  • Don't change the code or variable names.
  • All functions to should have concise accurate documentation with the exception of command "main" functions which are obviously entry points.
  • Document field structs but don't duplicate the field documentation above.
  • Avoid duplicate documentation, just pick the most appropriate place.
  • Large or complex blocks of code that aren't self documenting should be documented.
  • Avoid re-writing any existing documentation unless it's incomplete, or incorrect

Example of documentation that isn't useful and should be avoided:

// It uses JSON format at INFO level for structured logging.
var (
log = logger.MustNewLogger(false, configv1alpha1.InfoLevel, configv1alpha1.LogFormatJSON).WithName("grove-initc")
)```. 

Examples of documentation that doesn't add value: 
```  // Parse the replica count as an integer
replicas, err := strconv.Atoi(nameAndMinAvailable[1])
if err != nil {
 return nil, groveerr.WrapError(err, errCodeInvalidInput, operationParseFlag, "failed to convert replicas to int")
}
  // Store the PodClique name and its minimum available replicas
  podCliqueDependencies[strings.TrimSpace(nameAndMinAvailable[0])] = replicas
// Initialize the configuration with empty PodClique list
 config := CLIOptions{
  podCliques: make([]string, 0),
 }
 // Register all command line flags
 config.RegisterFlags(pflag.CommandLine)
 // Parse the command line arguments
 pflag.Parse()
 // Get the in-cluster REST configuration
 restConfig, err := rest.InClusterConfig()
// Get the pod informer from the factory
typedInformer := factory.Core().V1().Pods().Informer()

Systematically check every golang file under this directory is documented as per the instructions above.

Does this PR introduce a API change?

NONE

@gflarity gflarity force-pushed the gflarity/document_internal_controller branch from c5a38f5 to 7344e2c Compare September 29, 2025 19:04

@renormalize renormalize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like many functions in operator/internal/controller/podcliqueset/ were not documented.

If this PR intends to make the doc strings exhaustive for all functions in internal/controller, then can the remaining functions be documented as well? Thanks.

Comment thread operator/internal/controller/podclique/reconcilespec.go Outdated
Comment thread operator/internal/controller/podclique/reconcilestatus.go Outdated
Comment thread operator/internal/controller/podclique/reconcilestatus.go Outdated
@gflarity gflarity requested a review from renormalize October 22, 2025 19:05
@gflarity

gflarity commented Oct 22, 2025

Copy link
Copy Markdown
Contributor Author

@renormalize, thanks for catching the missing documentation. I've added more specific steps to verify there's no missing documentation into prompt. I'm now using Claude 4.5 Sonnet Thinking and this updated prompt:


Improve the inline documentation for for all golang files under this directory/package. Here are the rules to follow:

  • Use concise idiomatic go documentation principles.
  • Ensure correctness.
  • Add some minimal inline documentation above complex code blocks when to explain the intent of the code.
  • Don't document code that is clearly self documenting.
  • Blocks of code should only have a comment describing them if it is practical and valuable.
  • Don't change the code or variable names.
  • All functions to should have concise accurate documentation with the exception of command "main" functions which are obviously entry points.
  • Document field structs but don't duplicate the field documentation above.
  • Avoid duplicate documentation, just pick the most appropriate place.
  • Large or complex blocks of code that aren't self documenting should be documented.
  • Avoid re-writing any existing documentation unless it's incomplete, or incorrect

Example of documentation that isn't useful and should be avoided:

// It uses JSON format at INFO level for structured logging.
var (
log = logger.MustNewLogger(false, configv1alpha1.InfoLevel, configv1alpha1.LogFormatJSON).WithName("grove-initc")
)```. 

Examples of documentation that doesn't add value: 
```  // Parse the replica count as an integer
replicas, err := strconv.Atoi(nameAndMinAvailable[1])
if err != nil {
 return nil, groveerr.WrapError(err, errCodeInvalidInput, operationParseFlag, "failed to convert replicas to int")
}
  // Store the PodClique name and its minimum available replicas
  podCliqueDependencies[strings.TrimSpace(nameAndMinAvailable[0])] = replicas
// Initialize the configuration with empty PodClique list
 config := CLIOptions{
  podCliques: make([]string, 0),
 }
 // Register all command line flags
 config.RegisterFlags(pflag.CommandLine)
 // Parse the command line arguments
 pflag.Parse()
 // Get the in-cluster REST configuration
 restConfig, err := rest.InClusterConfig()
// Get the pod informer from the factory
typedInformer := factory.Core().V1().Pods().Informer()

Systematically check every golang file under this directory is documented as per the instructions above.

Check every file, here's some useful tools:

List target directories

find -type d

Check for undocumented functions in specific package (run this again after you'd made changes to verify)

find -name ".go" -not -name "_test.go"
-exec grep -Hn "^func " {} ; |
grep -v "^//|main(" |
awk -F: '{print $1":"$2}' |
while read location; do
file=$(echo $location | cut -d: -f1)
line=$(echo $location | cut -d: -f2)
prev_line=$((line - 1))
if [ $prev_line -gt 0 ]; then
prev=$(sed -n "${prev_line}p" "$file")
if [[ ! "$prev" =~ ^// ]]; then
echo "$location"
fi
fi
done

How this command works:

  1. find - Locates all Go files (excluding tests)
  2. grep -Hn "^func " - Finds all function declarations with file:line numbers
  3. grep -v "^//\|main(" - Filters out commented lines and main functions
  4. awk - Extracts file and line information
  5. while read loop - Checks if the line before each function is a comment
  6. If no comment exists, outputs the location

@gflarity gflarity force-pushed the gflarity/document_internal_controller branch from df2c302 to 999a001 Compare October 22, 2025 19:26

@renormalize renormalize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for incorporating all the requested changes @gflarity!
Added 2 more comments, after which we can go ahead with the merge.

Also, thanks for updating the PR with the updated prompt used to generate the documentation!

PS: was unable to provide a suggested change in the comment since GitHub refuses to show that button to me for some reason :/

Comment thread operator/internal/controller/podcliqueset/components/podgang/syncflow.go Outdated
Comment thread operator/internal/controller/podcliqueset/components/podgang/syncflow.go Outdated
gflarity and others added 6 commits October 22, 2025 16:43
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
Co-authored-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Geoff Flarity <geoff.flarity@gmail.com>
Co-authored-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Geoff Flarity <geoff.flarity@gmail.com>
Co-authored-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Geoff Flarity <geoff.flarity@gmail.com>
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
@gflarity gflarity force-pushed the gflarity/document_internal_controller branch from 999a001 to 2ae1d55 Compare October 22, 2025 20:48
@gflarity gflarity requested a review from renormalize October 22, 2025 20:48
@gflarity

Copy link
Copy Markdown
Contributor Author

@renormalize Reverted those changes. PTAL

@renormalize renormalize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good! Thanks!

@gflarity gflarity merged commit c1ca47e into ai-dynamo:main Oct 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants