Skip to content

Comments

fix(cli): Improve cli wrapper and fix init command behavior#1214

Draft
creatorrr wants to merge 1 commit intodevfrom
x/fix-cli-init
Draft

fix(cli): Improve cli wrapper and fix init command behavior#1214
creatorrr wants to merge 1 commit intodevfrom
x/fix-cli-init

Conversation

@creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Mar 5, 2025

  • refactor(agents-api): Refactor text_to_tsquery -> text_to_keywords
  • feat(agents-api,migrations): Add trigram search to FTS
  • fix(agents-api): Remove too-long magic number
  • fix(agents-api): Renamed confusing is_conversation_snippet flag
  • fix(cli): Improve cli wrapper and fix init command behavior

EntelligenceAI PR Summary

Purpose:

  • Enhance full-text search and improve CLI modularity and usability.

Changes:

  • Enhancement: Refined keyword extraction in NLP for better search.
  • New Feature: Added template selection in CLI init command.
  • Refactor: Reorganized imports for improved code clarity.
  • Style: Implemented a new custom prompt style in CLI.

Impact:

  • Streamlined CLI experience, improved search features, and increased code manageability.

Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
@entelligence-ai-pr-reviews
Copy link
Contributor

Walkthrough

This PR enhances the NLP module by refining keyword extraction functions and introducing a new parameter for improved full-text search capabilities. The CLI has been refactored for better modularity, including new utility functions, a custom prompt style, and template selection support in the init command. Import reorganization across various files improves code clarity.

Changes

File(s) Summary
agents-api/agents_api/common/nlp.py Refactored text_to_tsvector_query to text_to_keywords, changing return type to a set for keyword extraction. Updated keyword filtering logic.
agents-api/agents_api/common/utils/get_doc_search.py Added extract_keywords parameter to get_search_fn_and_params for enhanced keyword extraction.
cli/src/julep_cli/init.py Enhanced init command with template selection and improved error handling.
cli/src/julep_cli/style.py Introduced custom style for CLI prompts.
cli/src/julep_cli/templates.py Added template fetching logic from GitHub repository.
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @bot + *your message*
Example: @bot Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @bot + *feedback*
Example: @bot Do not comment on `save_auth` function !

Comment on lines 153 to 156
keywords = extract_keywords(sent_doc, top_n, split_chunks=split_chunks)
keywords = [kw for kw in keywords if len(kw) > 1]
if len(keywords) < min_keywords:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Filtering keywords with len(kw) > 1 after extraction could return fewer keywords than min_keywords, making the minimum check ineffective. Move length check before the minimum check.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
keywords = extract_keywords(sent_doc, top_n, split_chunks=split_chunks)
keywords = [kw for kw in keywords if len(kw) > 1]
if len(keywords) < min_keywords:
continue
keywords = [kw for kw in extract_keywords(sent_doc, top_n, split_chunks=split_chunks) if len(kw) > 1]
if len(keywords) < min_keywords:
continue

@anasalatasiuni
Copy link
Contributor

@claude please review

@claude
Copy link
Contributor

claude bot commented Sep 1, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@anasalatasiuni
Copy link
Contributor

@claude please review

@claude
Copy link
Contributor

claude bot commented Sep 1, 2025

Claude finished @anasalatasiuni's task —— View job


📋 Code Review for PR #1214

I've completed my review of this PR that improves the CLI wrapper and fixes init command behavior. Here's my comprehensive feedback:

✅ Overall Assessment

The PR enhances the CLI's usability and modularity with template selection support, custom styling, and improved error handling. The refactoring improves code organization and maintainability.

🎯 Key Changes Reviewed

1. CLI Improvements

  • Template Selection System: Well-implemented template fetching from GitHub repository with proper caching via joblib.Memory
  • Custom Prompt Style: Good addition of consistent styling in style.py for better UX
  • Enhanced Wrapper: The WrappedTyper class with interactive prompts for missing parameters is a significant improvement

2. Code Quality Issues Found

🔧 Issue 1: Keyword Filtering Logic (agents-api/agents_api/common/nlp.py:154-156)

The existing review comment is valid. The current implementation filters keywords by length > 1 AFTER extraction, which could result in fewer keywords than min_keywords:

keywords = extract_keywords(sent_doc, top_n, split_chunks=split_chunks)
keywords = [kw for kw in keywords if len(kw) > 1]  # This happens after extraction
if len(keywords) < min_keywords:
    continue

Recommendation: Move the length check inside extract_keywords() or adjust the logic to ensure min_keywords is satisfied after filtering.

⚠️ Issue 2: Error Handling in Template Fetching (cli/src/julep_cli/templates.py:64-69)

The template fetching silently fails with only a warning message:

try:
    templates = fetch_library_templates()
except (GithubException, TimeoutError) as _e:
    console.print(Text("Failed to fetch library templates", style="italic yellow"))

Recommendation: Consider providing a fallback mechanism or at least setting templates to a default value to prevent potential issues when templates are accessed later.

💡 Issue 3: Hardcoded Timeout Values (cli/src/julep_cli/templates.py:28-31)

The GitHub client has hardcoded timeout values that might be too restrictive:

gh = Github(
    auth=Auth.Token(GH_ACCESS_TOKEN) if GH_ACCESS_TOKEN else None,
    lazy=True,
    timeout=3,  # Very short timeout
    retry=None,
)

Recommendation: Consider making these configurable or increasing the timeout for better reliability.

🚀 Improvements & Suggestions

  1. Import Organization: The import reorganization across files is clean and consistent ✓

  2. Path Handling: Good use of Path objects and proper relative/absolute path conversions in to_relative() function

  3. User Experience: The confirmation prompts and progress indicators enhance the CLI experience

  4. Dependency Management: New dependencies (pygithub, joblib, questionary) are appropriate for the added functionality

📝 Minor Observations

  • The DocStrEnum class is a clever way to add documentation to enum values
  • Good use of @lru_cache for performance optimization in NLP functions
  • The custom style definition provides a consistent theme across the CLI

✅ Review Checklist

  • Check for CLAUDE.md and repo-specific instructions
  • Review CLI changes (init.py, wrapper.py, templates.py, style.py)
  • Review NLP refactoring (if included in this PR)
  • Review utility and import reorganization changes
  • Check for potential issues and improvements
  • Provide comprehensive feedback

🎬 Summary

The PR makes solid improvements to the CLI's usability and code organization. The main concerns are the keyword filtering logic issue (already noted by another reviewer) and the error handling in template fetching. With these minor adjustments, this PR will significantly enhance the Julep CLI experience.


View job

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.

2 participants