Skip to content

XAssist - add model freedom and ollama support #156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

tharun571
Copy link
Contributor

@tharun571 tharun571 commented Sep 27, 2024

Description

  • Users can now select and use a specific model within their respective LLM.
  • Users can set the URL for Ollama and can add a model and utilize the LLM.
  • Updated the documentation to reflect the new features and changes.
  • Added tests for the new Ollama support.

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@tharun571 tharun571 marked this pull request as draft September 27, 2024 15:06
@tharun571 tharun571 changed the title Model freedom and Ollama support XAssist - add model freedom and ollama support Sep 27, 2024
@tharun571 tharun571 marked this pull request as ready for review September 27, 2024 15:59
@anutosh491
Copy link
Collaborator

The failing CI (installing mamba/ micromamba) is due to the new release
Pinning the version should fix it

- uses: mamba-org/setup-micromamba@v1
  with:
    micromamba-version: '1.5.10-0'

Let's do this for now, untill the issue is fixed

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 47.61905% with 44 lines in your changes missing coverage. Please review.

Project coverage is 75.26%. Comparing base (28eed52) to head (d9c7c1e).

Files with missing lines Patch % Lines
src/xmagics/xassist.cpp 47.61% 44 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
- Coverage   81.97%   75.26%   -6.71%     
==========================================
  Files          19       19              
  Lines         760      837      +77     
  Branches       79       91      +12     
==========================================
+ Hits          623      630       +7     
- Misses        137      207      +70     
Files with missing lines Coverage Δ
src/xmagics/xassist.cpp 54.28% <47.61%> (-26.17%) ⬇️
Files with missing lines Coverage Δ
src/xmagics/xassist.cpp 54.28% <47.61%> (-26.17%) ⬇️

std::cerr << "Failed to open file for reading URL for model " << model << std::endl;
return "";
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically any utility function like these should be tested for (either in test_interpreter.cpp itself or maybe through a test_utils.cpp if possible)

But that can be done through subsequent PRs (an issue can be opened to track the same) !

Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

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

Clang-tidy reviews should be addressed. Apart from that it LGTM.

I would just wait for the green flag by @vgvassilev before merging !

@tharun571 tharun571 force-pushed the xassist branch 2 times, most recently from 1de9a78 to e28d80a Compare October 8, 2024 15:06
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM, let's ignore the infrastructure issue of the clang-tidy bot.

@vgvassilev vgvassilev merged commit 87e0ab2 into compiler-research:main Oct 12, 2024
9 of 10 checks passed
@tharun571 tharun571 mentioned this pull request Oct 16, 2024
3 tasks
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.

4 participants