Skip to content

Conversation

@jcortes
Copy link
Collaborator

@jcortes jcortes commented May 23, 2025

WHY

Resolves #16731

Summary by CodeRabbit

  • New Features

    • Added actions to upload datasets, create fine-tune jobs, evaluate jobs, generate text completions, and retrieve batch completions for Lamini models.
    • Introduced a source component to monitor and emit training job status updates.
    • Enhanced Lamini app integration with dynamic model and job selectors, structured output configuration, file download support, and improved API communication.
    • Added utility for recursive JSON parsing and centralized API constants.
  • Chores

    • Updated package version and added a dependency on @pipedream/platform.

@jcortes jcortes self-assigned this May 23, 2025
@vercel
Copy link

vercel bot commented May 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs-v2 ⬜️ Ignored (Inspect) Visit Preview May 28, 2025 5:27pm
pipedream-docs ⬜️ Ignored (Inspect) May 28, 2025 5:27pm
pipedream-docs-redirect-do-not-edit ⬜️ Ignored (Inspect) May 28, 2025 5:27pm

@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 23, 2025

Walkthrough

This update introduces a new Lamini integration featuring five actions and one polling source. The actions enable uploading datasets, creating and evaluating fine-tune jobs, generating completions, and handling batch completions. The polling source emits events on fine-tuning job status. Supporting utility and constants modules, as well as app-level API and property definitions, are also included.

Changes

Files/Paths Change Summary
components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs New action: create fine-tune job with various parameters and Lamini API integration.
components/lamini/actions/evaluate-job/evaluate-job.mjs New action: evaluate a fine-tuning job by job ID.
components/lamini/actions/generate-completion/generate-completion.mjs New action: generate text completion with a Lamini model.
components/lamini/actions/get-batch-completions/get-batch-completions.mjs New action: submit and retrieve batch completion requests.
components/lamini/actions/upload-dataset/upload-dataset.mjs New action: upload dataset for training, with validation and transformation.
components/lamini/common/constants.mjs New module: centralizes API base URL and version paths.
components/lamini/common/utils.mjs New utility: recursive JSON parsing function.
components/lamini/lamini.app.mjs New app: defines prop definitions, API methods, and file download utility for Lamini integration.
components/lamini/package.json Version bump to 0.1.0; adds dependency on @pipedream/platform.
components/lamini/sources/get-job-status/get-job-status.mjs New polling source: emits events with Lamini fine-tuning job status.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant LaminiApp
    participant LaminiAPI

    User->>LaminiApp: Upload Dataset (fileUrl, inputKey, ...)
    LaminiApp->>LaminiAPI: POST /datasets/upload
    LaminiAPI-->>LaminiApp: Dataset ID
    LaminiApp-->>User: Dataset upload summary

    User->>LaminiApp: Create Fine-Tune Job (modelName, datasetId, ...)
    LaminiApp->>LaminiAPI: POST /train
    LaminiAPI-->>LaminiApp: Fine-tune Job ID
    LaminiApp-->>User: Fine-tune job summary

    User->>LaminiApp: Evaluate Job (jobId)
    LaminiApp->>LaminiAPI: GET /finetune_eval/jobs/{jobId}
    LaminiAPI-->>LaminiApp: Evaluation result
    LaminiApp-->>User: Evaluation summary

    User->>LaminiApp: Generate Completion (modelName, prompt, ...)
    LaminiApp->>LaminiAPI: POST /completions
    LaminiAPI-->>LaminiApp: Completion result
    LaminiApp-->>User: Completion summary

    User->>LaminiApp: Get Batch Completions (modelName, prompts, ...)
    LaminiApp->>LaminiAPI: POST /batch_completions
    LaminiAPI-->>LaminiApp: Batch ID
    LaminiApp->>LaminiAPI: GET /batch_completions/{id}/result
    LaminiAPI-->>LaminiApp: Batch completion results
    LaminiApp-->>User: Batch completion summary

    Note over LaminiApp, LaminiAPI: Polling Source
    LaminiApp->>LaminiAPI: GET /jobs/{jobId} (periodic)
    LaminiAPI-->>LaminiApp: Job status
    LaminiApp-->>User: Emit job status event
Loading

Assessment against linked issues

Objective Addressed Explanation
Emit event with status of Lamini fine-tuning job (get-job-status source) (#16731)
Upload dataset for training (upload-dataset action) with required and optional params (#16731)
Create fine-tuning job with required and optional params (create-fine-tune-job action) (#16731)
Evaluate fine-tuning job by job ID (evaluate-job action) (#16731)
Retrieve batch completion results and generate completions with required/optional params (get-batch-completions, generate-completion) (#16731)

Poem

A rabbit hopped with code so bright,
Bringing Lamini actions to light.
Upload, fine-tune, and batch away—
Completions bloom without delay!
With polling hops for job review,
The warren cheers this version new.
🐇✨

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

components/lamini/actions/get-batch-completions/get-batch-completions.mjs

Oops! Something went wrong! :(

ESLint: 8.57.1

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
at Object.getPackageJSONURL (node:internal/modules/package_json_reader:255:9)
at packageResolve (node:internal/modules/esm/resolve:767:81)
at moduleResolve (node:internal/modules/esm/resolve:853:18)
at defaultResolve (node:internal/modules/esm/resolve:983:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:799:12)
at #cachedDefaultResolve (node:internal/modules/esm/loader:723:25)
at ModuleLoader.resolve (node:internal/modules/esm/loader:706:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:307:38)
at #link (node:internal/modules/esm/module_job:170:49)

components/lamini/actions/generate-completion/generate-completion.mjs

Oops! Something went wrong! :(

ESLint: 8.57.1

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
at Object.getPackageJSONURL (node:internal/modules/package_json_reader:255:9)
at packageResolve (node:internal/modules/esm/resolve:767:81)
at moduleResolve (node:internal/modules/esm/resolve:853:18)
at defaultResolve (node:internal/modules/esm/resolve:983:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:799:12)
at #cachedDefaultResolve (node:internal/modules/esm/loader:723:25)
at ModuleLoader.resolve (node:internal/modules/esm/loader:706:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:307:38)
at #link (node:internal/modules/esm/module_job:170:49)

components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs

Oops! Something went wrong! :(

ESLint: 8.57.1

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
at Object.getPackageJSONURL (node:internal/modules/package_json_reader:255:9)
at packageResolve (node:internal/modules/esm/resolve:767:81)
at moduleResolve (node:internal/modules/esm/resolve:853:18)
at defaultResolve (node:internal/modules/esm/resolve:983:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:799:12)
at #cachedDefaultResolve (node:internal/modules/esm/loader:723:25)
at ModuleLoader.resolve (node:internal/modules/esm/loader:706:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:307:38)
at #link (node:internal/modules/esm/module_job:170:49)

  • 6 others

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac9cce1 and 03cb8f3.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (1 hunks)
  • components/lamini/actions/evaluate-job/evaluate-job.mjs (1 hunks)
  • components/lamini/actions/generate-completion/generate-completion.mjs (1 hunks)
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs (1 hunks)
  • components/lamini/actions/upload-dataset/upload-dataset.mjs (1 hunks)
  • components/lamini/common/constants.mjs (1 hunks)
  • components/lamini/common/utils.mjs (1 hunks)
  • components/lamini/lamini.app.mjs (1 hunks)
  • components/lamini/package.json (2 hunks)
  • components/lamini/sources/get-job-status/get-job-status.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • components/lamini/package.json
  • components/lamini/common/constants.mjs
  • components/lamini/common/utils.mjs
  • components/lamini/actions/evaluate-job/evaluate-job.mjs
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs
  • components/lamini/sources/get-job-status/get-job-status.mjs
  • components/lamini/actions/generate-completion/generate-completion.mjs
  • components/lamini/actions/upload-dataset/upload-dataset.mjs
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Publish TypeScript components
  • GitHub Check: pnpm publish
  • GitHub Check: Verify TypeScript components
  • GitHub Check: Lint Code Base
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (4)
components/lamini/actions/generate-completion/generate-completion.mjs (1)

51-74: Consider adding error handling and input validation.

The run method lacks error handling for the API call and doesn't validate required inputs before making the request.

Consider adding basic validation and error handling:

  async run({ $ }) {
    const {
      generateCompletion,
      modelName,
      prompt,
      outputType,
      maxTokens,
      maxNewTokens,
    } = this;

+   if (!prompt?.trim()) {
+     throw new ConfigurationError("Prompt is required and cannot be empty.");
+   }

-   const response = await generateCompletion({
+   try {
+     const response = await generateCompletion({
      $,
      data: {
        model_name: modelName,
        prompt,
        output_type: utils.parseJson(outputType),
        max_tokens: maxTokens,
        max_new_tokens: maxNewTokens,
      },
-   });
+     });

    $.export("$summary", `Successfully generated completion for prompt with model ${modelName}.`);
    return response;
+   } catch (error) {
+     throw new ConfigurationError(`Failed to generate completion: ${error.message}`);
+   }
  },
components/lamini/actions/upload-dataset/upload-dataset.mjs (2)

10-10: Version inconsistency with other components.

This component is at version 0.0.6 while other components in this PR are at 0.0.1. Consider aligning versions for consistency.


68-70: File format validation could be more robust.

The current validation only checks file extensions, not actual content format. Consider validating the actual JSON Lines format.

Consider adding content-based validation:

  if (!fileName.endsWith(".jsonl") && !fileName.endsWith(".jsonlines")) {
    throw new ConfigurationError(`Unsupported file format for \`${fileName}\`. Only **.jsonl** and **.jsonlines** files are supported.`);
  }

+ // Validate that file contains valid JSON Lines format
+ const fileContent = fs.readFileSync(filePath, "utf8");
+ const lines = fileContent.trim().split("\n");
+ 
+ if (lines.length === 0) {
+   throw new ConfigurationError(`File ${fileName} is empty or contains no valid lines.`);
+ }
components/lamini/lamini.app.mjs (1)

138-161: Consider adding error handling for file operations

The downloadFileToTmp method should handle potential file system errors to provide better error messages to users.

 async downloadFileToTmp({
   $, url,
 }) {
   const fileName = url.split("/").pop();

   const resp = await this.makeRequest({
     $,
     url,
     responseType: "arraybuffer",
     headers: {
       noHeaders: true,
     },
   });
   const rawcontent = resp.toString("base64");
   const buffer = Buffer.from(rawcontent, "base64");
   const filePath = `/tmp/${fileName}`;

-  fs.writeFileSync(filePath, buffer);
+  try {
+    fs.writeFileSync(filePath, buffer);
+  } catch (error) {
+    throw new ConfigurationError(`Failed to write file to temporary directory: ${error.message}`);
+  }

   return {
     fileName,
     filePath,
   };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd1cd42 and c60d3b7.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (1 hunks)
  • components/lamini/actions/evaluate-job/evaluate-job.mjs (1 hunks)
  • components/lamini/actions/generate-completion/generate-completion.mjs (1 hunks)
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs (1 hunks)
  • components/lamini/actions/upload-dataset/upload-dataset.mjs (1 hunks)
  • components/lamini/common/constants.mjs (1 hunks)
  • components/lamini/common/utils.mjs (1 hunks)
  • components/lamini/lamini.app.mjs (1 hunks)
  • components/lamini/package.json (2 hunks)
  • components/lamini/sources/get-job-status/get-job-status.mjs (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: pnpm publish
  • GitHub Check: Publish TypeScript components
  • GitHub Check: Verify TypeScript components
  • GitHub Check: Lint Code Base
🔇 Additional comments (13)
components/lamini/common/constants.mjs (1)

1-14: LGTM! Well-structured constants module.

The constants are well-organized with a clear separation between base URL and version paths. The multiple API versions indicate good forward compatibility planning, and the export structure follows ES6 module best practices.

components/lamini/package.json (2)

3-3: Appropriate version bump for new features.

The version bump from "0.0.1" to "0.1.0" correctly follows semantic versioning conventions for a minor release that introduces new functionality.


15-17:

✅ Verification successful

Verify the dependency version is current.

Ensure that the "@pipedream/platform" version "^3.0.3" is the latest stable version available.


🌐 Web query:

What is the latest version of @pipedream/platform npm package?

💡 Result:

The latest version of the @pipedream/platform npm package is 3.0.3. This version was published approximately 7-8 months ago according to the first search result, though another result indicates it was published 16 days ago (which likely refers to the last update to the package page rather than the package itself)[1][2]. The package provides Pipedream platform globals for typing and runtime type checking and is available through npm by installing it with the command npm i @pipedream/platform[1].

Pipedream is a serverless integration and compute platform that makes it easy to connect apps and develop event-driven workflows. The platform supports over 300 fully integrated applications with managed authentication and offers access to over 1 million npm packages[5], allowing developers to easily integrate various APIs and services.

Citations:


Dependency version is up-to-date

The @pipedream/platform entry in components/lamini/package.json is already set to ^3.0.3, which matches the latest stable release on npm. No update is needed.

components/lamini/actions/evaluate-job/evaluate-job.mjs (1)

1-43: LGTM! Well-implemented Pipedream action.

The action follows Pipedream conventions correctly:

  • Proper import structure and metadata definition
  • Good use of propDefinition for reusable job ID property
  • Clean separation of concerns with dedicated evaluateJob method
  • Appropriate async handling and user feedback via $.export
  • Correct API path construction using template literals

The implementation demonstrates good practices for Pipedream component development.

components/lamini/actions/generate-completion/generate-completion.mjs (1)

1-76: LGTM! Well-structured Pipedream action component.

The component follows Pipedream conventions well with proper imports, prop definitions, and API integration. The use of shared app prop definitions promotes consistency across the integration.

components/lamini/sources/get-job-status/get-job-status.mjs (1)

30-38: LGTM! Proper event emission pattern.

The processEvent method correctly implements the Pipedream event emission pattern with unique IDs, summary, and timestamps.

components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (3)

34-44: Excellent comprehensive documentation for complex parameters.

The detailed descriptions for fineTuneArgs and gpuConfig with specific parameter examples and documentation links provide excellent user guidance for these complex configuration objects.


59-66: Good use of API versioning.

The implementation correctly uses the version path constant for API versioning, which is a best practice for API integrations.


80-91: Proper data transformation and null handling.

The data object construction correctly transforms camelCase to snake_case and uses utils.parseJson for optional JSON parameters, with proper null handling.

components/lamini/actions/upload-dataset/upload-dataset.mjs (2)

79-95: Good error handling for JSON parsing and required fields.

The error handling appropriately catches JSON parsing errors, validates required input keys, and provides clear error messages. The logic to skip invalid JSON lines while preserving critical validation errors is well implemented.


97-99: Appropriate validation for empty datasets.

Good validation to ensure at least some valid data exists before proceeding with upload.

components/lamini/actions/get-batch-completions/get-batch-completions.mjs (2)

4-42: Props definition looks good!

The component metadata and props are well-structured with appropriate types and descriptions. The use of propDefinition for shared properties promotes consistency across actions.


58-88: Run method implementation is well-structured!

The async flow properly submits the batch completion request and retrieves results using the returned ID. The summary message provides clear feedback to users.

@jcortes jcortes force-pushed the lamini-new-components branch from c60d3b7 to e8839b8 Compare May 23, 2025 22:35
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (1)
components/lamini/lamini.app.mjs (1)

130-137: Method reference issue has been correctly fixed.

The previous issue with this.app.makeRequest() has been properly resolved - line 133 now correctly calls this.makeRequest() within the app context.

🧹 Nitpick comments (1)
components/lamini/lamini.app.mjs (1)

96-102: Enhance error handling for better API diagnostics.

While the status 424 handling is good, consider adding more comprehensive error handling for other common API issues.

     } catch (error) {
       if (error.status === 424 && error.data === "") {
         console.log("API is not ready yet. Retrying...", error);
         throw new ConfigurationError("The API is not ready yet. Please try again later.");
       }
+      if (error.status === 401) {
+        throw new ConfigurationError("Invalid API key. Please check your authentication credentials.");
+      }
+      if (error.status === 403) {
+        throw new ConfigurationError("Access forbidden. Please verify your API permissions.");
+      }
       throw error;
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c60d3b7 and e8839b8.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (1 hunks)
  • components/lamini/actions/evaluate-job/evaluate-job.mjs (1 hunks)
  • components/lamini/actions/generate-completion/generate-completion.mjs (1 hunks)
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs (1 hunks)
  • components/lamini/actions/upload-dataset/upload-dataset.mjs (1 hunks)
  • components/lamini/common/constants.mjs (1 hunks)
  • components/lamini/common/utils.mjs (1 hunks)
  • components/lamini/lamini.app.mjs (1 hunks)
  • components/lamini/package.json (2 hunks)
  • components/lamini/sources/get-job-status/get-job-status.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • components/lamini/common/constants.mjs
  • components/lamini/actions/evaluate-job/evaluate-job.mjs
  • components/lamini/package.json
  • components/lamini/actions/upload-dataset/upload-dataset.mjs
  • components/lamini/sources/get-job-status/get-job-status.mjs
  • components/lamini/actions/generate-completion/generate-completion.mjs
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: pnpm publish
  • GitHub Check: Verify TypeScript components
  • GitHub Check: Publish TypeScript components
  • GitHub Check: Lint Code Base
🔇 Additional comments (2)
components/lamini/common/utils.mjs (1)

1-30: Excellent fixes applied - infinite recursion and circular reference issues resolved.

The implementation correctly addresses all the critical issues mentioned in the previous review:

Fixed infinite recursion: Line 9 now correctly calls parseJson(JSON.parse(value), maxDepth - 1) instead of the problematic recursive call
Added circular reference protection: Uses WeakSet to track seen objects (lines 2, 14-16)
Implemented recursion depth limiting: maxDepth parameter with proper decrementing (lines 1, 4-6, 9)

The design is well-thought-out: creating new WeakSet contexts for JSON string parsing while reusing contexts for object traversal is the correct approach for handling different object graphs.

components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (1)

86-87:

❓ Verification inconclusive

Clarify the parseJson usage on object properties.

The fineTuneArgs and gpuConfig props are defined as type "object" but then passed through utils.parseJson(). This raises questions about the expected data format:

  • If these props are meant to receive JSON strings, they should be defined as type "string"
  • If they're already objects, the parseJson calls might be unnecessary unless they contain nested JSON strings

Please clarify the intended usage or add comments explaining why object props need JSON parsing.


🏁 Script executed:

#!/bin/bash
# Description: Check how these object props are used in other Lamini actions
# Expected: Find similar patterns or different approaches

echo "Searching for fineTuneArgs usage patterns:"
rg -A 3 -B 3 "fineTuneArgs|gpuConfig" --type js

echo -e "\nSearching for parseJson usage on object props:"
rg -A 2 -B 2 "parseJson.*\)" --type js

Length of output: 294


Clarify utils.parseJson usage on object props

The fineTuneArgs and gpuConfig props are defined as object but then passed through utils.parseJson(), which typically parses JSON strings. Please verify and adjust:

  • If these props should accept JSON strings, change their prop type to string.
  • If they should accept objects, remove or guard the parseJson calls.
  • Add inline validation or schema checks for required fields within fineTuneArgs and gpuConfig.

Diff suggestion:

- finetune_args: utils.parseJson(fineTuneArgs),
- gpu_config:    utils.parseJson(gpuConfig),
+ finetune_args: fineTuneArgs,
+ gpu_config:    gpuConfig,

lcaresia
lcaresia previously approved these changes May 26, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
components/lamini/lamini.app.mjs (2)

25-32: Add null safety for model name mapping.

The code assumes custom_model_name exists but it could be null or undefined, which would result in incorrect option labels.

Apply this diff to add null safety:

     const tunedModels = jobs.filter(({ status }) => status === "COMPLETED")
       .map(({
         custom_model_name: label,
         model_name: value,
       }) => ({
-        label,
+        label: label || value,
         value,
       }));

This ensures the label falls back to the model name if no custom name is provided.


162-166: Fix incorrect buffer handling in downloadFileToTmp method.

The buffer handling logic is incorrect and will likely cause issues:

  1. Line 162: resp.toString("base64") assumes resp is a string/buffer, but with responseType: "arraybuffer", resp is the entire axios response object
  2. Line 163: Converting the string back to Buffer is unnecessary

Apply this diff to fix the buffer handling:

-    const rawcontent = resp.toString("base64");
-    const buffer = Buffer.from(rawcontent, "base64");
+    const buffer = Buffer.from(resp.data);

The resp.data contains the actual ArrayBuffer from the response.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8839b8 and 4c6ac62.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (1 hunks)
  • components/lamini/actions/evaluate-job/evaluate-job.mjs (1 hunks)
  • components/lamini/actions/generate-completion/generate-completion.mjs (1 hunks)
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs (1 hunks)
  • components/lamini/actions/upload-dataset/upload-dataset.mjs (1 hunks)
  • components/lamini/common/constants.mjs (1 hunks)
  • components/lamini/common/utils.mjs (1 hunks)
  • components/lamini/lamini.app.mjs (1 hunks)
  • components/lamini/package.json (2 hunks)
  • components/lamini/sources/get-job-status/get-job-status.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • components/lamini/package.json
  • components/lamini/common/constants.mjs
  • components/lamini/common/utils.mjs
  • components/lamini/actions/generate-completion/generate-completion.mjs
  • components/lamini/actions/evaluate-job/evaluate-job.mjs
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs
  • components/lamini/actions/upload-dataset/upload-dataset.mjs
  • components/lamini/sources/get-job-status/get-job-status.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Verify TypeScript components
  • GitHub Check: Publish TypeScript components
  • GitHub Check: pnpm publish
  • GitHub Check: Lint Code Base
🔇 Additional comments (1)
components/lamini/lamini.app.mjs (1)

95-101: Good error handling for API readiness.

The error handling for 424 status with empty response body appropriately catches API readiness issues and provides a clear error message to users.

@jcortes jcortes force-pushed the lamini-new-components branch from 37ca970 to ac9cce1 Compare May 28, 2025 15:26
@jcortes
Copy link
Collaborator Author

jcortes commented May 28, 2025

/approve

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (3)
components/lamini/lamini.app.mjs (3)

25-32: Add null safety for model name mapping.

The code assumes custom_model_name exists but it could be null or undefined, which would result in incorrect option labels.

This is the same issue identified in previous review comments.


46-50: Add null safety for job label mapping.

Similar to the modelName property, the jobId options mapping assumes custom_model_name exists, which could result in undefined labels.

This is the same issue identified in previous review comments.


162-166: Fix incorrect buffer handling in downloadFileToTmp method.

The buffer handling logic is incorrect and will likely cause issues.

This is the same issue identified in previous review comments regarding incorrect handling of axios response with arraybuffer responseType.

🧹 Nitpick comments (1)
components/lamini/lamini.app.mjs (1)

16-35: Add error handling for async options methods.

The async options methods should handle potential API errors gracefully to provide better user experience.

Consider wrapping the API calls in try-catch blocks:

 async options({ includeFineTunedModels = true }) {
+  try {
     const models = await this.listDownloadedModels();
     const hfModels = models.map(({ model_name: modelName }) => modelName);

     if (!includeFineTunedModels) {
       return hfModels;
     }

     const jobs = await this.listTrainedJobs();
     const tunedModels = jobs.filter(({ status }) => status === "COMPLETED")
       .map(({
         custom_model_name: label,
         model_name: value,
       }) => ({
         label,
         value,
       }));

     return hfModels.concat(tunedModels);
+  } catch (error) {
+    console.error("Error loading model options:", error);
+    return [];
+  }
 },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37ca970 and ac9cce1.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (10)
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs (1 hunks)
  • components/lamini/actions/evaluate-job/evaluate-job.mjs (1 hunks)
  • components/lamini/actions/generate-completion/generate-completion.mjs (1 hunks)
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs (1 hunks)
  • components/lamini/actions/upload-dataset/upload-dataset.mjs (1 hunks)
  • components/lamini/common/constants.mjs (1 hunks)
  • components/lamini/common/utils.mjs (1 hunks)
  • components/lamini/lamini.app.mjs (1 hunks)
  • components/lamini/package.json (2 hunks)
  • components/lamini/sources/get-job-status/get-job-status.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • components/lamini/package.json
  • components/lamini/common/constants.mjs
  • components/lamini/common/utils.mjs
  • components/lamini/actions/generate-completion/generate-completion.mjs
  • components/lamini/sources/get-job-status/get-job-status.mjs
  • components/lamini/actions/create-fine-tune-job/create-fine-tune-job.mjs
  • components/lamini/actions/upload-dataset/upload-dataset.mjs
  • components/lamini/actions/evaluate-job/evaluate-job.mjs
  • components/lamini/actions/get-batch-completions/get-batch-completions.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Verify TypeScript components
  • GitHub Check: pnpm publish
  • GitHub Check: Publish TypeScript components
  • GitHub Check: Lint Code Base

@jcortes jcortes force-pushed the lamini-new-components branch from ac9cce1 to 03cb8f3 Compare May 28, 2025 17:27
@jcortes jcortes merged commit 34a2c4e into master May 28, 2025
11 checks passed
@jcortes jcortes deleted the lamini-new-components branch May 28, 2025 17:56
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.

[Components] lamini

4 participants