Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Apr 21, 2025

User description

fixes issues with data logging for a/b testing with line profiler optimization candidates.


PR Type

Enhancement


Description

  • support separate control and experiment flows

  • add exp_type tracking to trace IDs

  • route profiling calls with experiment metadata

  • select AI service client per experiment group


Changes walkthrough 📝

Relevant files
Enhancement
function_optimizer.py
Add A/B experiment tracking in optimizer                                 

codeflash/optimization/function_optimizer.py

  • split control & experiment loops with exp_type labels
  • pass exp_type through optimization & logging calls
  • route profiling to appropriate AI service client
  • include experiment metadata in profiling requests
  • +16/-16 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    github-actions bot commented Apr 21, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 959b8af)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Overwrite Logic

    The loop iterating over control and experiment candidate sets reassigns best_optimization each time and ultimately returns only the last group’s result, discarding the other. Ensure both flows are handled or merged appropriately instead of overwriting.

    for _u, (candidates, exp_type) in enumerate(zip([optimizations_set.control, optimizations_set.experiment],["EXP0","EXP1"])):
        if candidates is None:
            continue
    Fragile Trace ID

    Constructing trace IDs by slicing with self.function_trace_id[:-4] + exp_type is brittle if the ID length varies or suffix conventions change. Consider a safer method to derive base IDs and append experiment types.

    ph("cli-optimize-function-finished", {"function_trace_id": self.function_trace_id[:-4] + exp_type if self.experiment_id else self.function_trace_id})

    @github-actions
    Copy link

    github-actions bot commented Apr 21, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 959b8af
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    centralize trace id logic

    Extract inline trace_id construction into a dedicated _build_trace_id helper method
    to reduce duplication.

    codeflash/optimization/function_optimizer.py [257]

    -ph("cli-optimize-function-finished", {"function_trace_id": self.function_trace_id[:-4] + exp_type if self.experiment_id else self.function_trace_id})
    +ph("cli-optimize-function-finished", {"function_trace_id": self._build_trace_id(exp_type)})
    Suggestion importance[1-10]: 6

    __

    Why: Extracting trace ID construction into a helper method reduces duplication across multiple call sites and centralizes the logic for easier maintenance.

    Low
    abstract client selection

    Encapsulate client selection into a helper method to improve readability and
    testability.

    codeflash/optimization/function_optimizer.py [382]

    -ai_service_client = self.aiservice_client if exp_type=="EXP0" else self.local_aiservice_client
    +ai_service_client = self._get_ai_service_client(exp_type)
    Suggestion importance[1-10]: 5

    __

    Why: Moving the AI client selection into a dedicated helper method enhances readability and makes it easier to unit‑test client resolution logic.

    Low
    Possible issue
    safe trace_id generation

    Use rsplit to safely remove the existing suffix rather than hard‐coded slicing.

    codeflash/optimization/function_optimizer.py [257]

    -self.function_trace_id[:-4] + exp_type if self.experiment_id else self.function_trace_id
    +self.function_trace_id.rsplit("_", 1)[0] + "_" + exp_type if self.experiment_id else self.function_trace_id
    Suggestion importance[1-10]: 5

    __

    Why: Using rsplit prevents issues if the suffix format or length changes, making trace ID handling more robust than hard‑coded slicing.

    Low

    Previous suggestions

    Suggestions up to commit 1333c8f
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use removesuffix for safety

    Replace hardcoded slicing with str.removesuffix to drop the previous suffix safely
    when generating the new trace ID.

    codeflash/optimization/function_optimizer.py [387]

    -trace_id=self.function_trace_id[:-4] + exp_type if self.experiment_id else self.function_trace_id,
    +trace_id=(self.function_trace_id.removesuffix(self.previous_exp_suffix) + exp_type) if self.experiment_id else self.function_trace_id,
    Suggestion importance[1-10]: 7

    __

    Why: Replacing hardcoded slicing with str.removesuffix makes the suffix removal more robust and clearer.

    Medium
    General
    Centralize trace ID logic

    Extract and reuse a helper to compute the experimental trace ID instead of repeating
    the slicing logic in every ph call.

    codeflash/optimization/function_optimizer.py [257]

    -ph("cli-optimize-function-finished", {"function_trace_id": self.function_trace_id[:-4] + exp_type if self.experiment_id else self.function_trace_id})
    +transformed_trace_id = self._get_experiment_trace_id(exp_type)
    +ph("cli-optimize-function-finished", {"function_trace_id": transformed_trace_id})
    Suggestion importance[1-10]: 6

    __

    Why: Extracting the trace ID computation into a helper avoids repeating the slicing logic and improves maintainability.

    Low
    Replace magic strings with constants

    Define EXP0 and EXP1 as named constants or an enum to avoid magic strings and
    improve readability.

    codeflash/optimization/function_optimizer.py [245]

    -for _u, (candidates, exp_type) in enumerate(zip([optimizations_set.control, optimizations_set.experiment],["EXP0","EXP1"])):
    +for _u, (candidates, exp_type) in enumerate(zip([optimizations_set.control, optimizations_set.experiment],[ExperimentGroup.CONTROL, ExperimentGroup.EXPERIMENT])):
    Suggestion importance[1-10]: 5

    __

    Why: Defining EXP0 and EXP1 as constants or an enum removes magic strings and improves code readability.

    Low

    @misrasaurabh1 misrasaurabh1 marked this pull request as ready for review April 21, 2025 21:16
    @aseembits93 aseembits93 merged commit c29d5fc into main Apr 21, 2025
    16 of 17 checks passed
    @github-actions
    Copy link

    Persistent review updated to latest commit 959b8af

    @aseembits93 aseembits93 deleted the cf-600 branch April 23, 2025 19:33
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants