Skip to content

Conversation

2b3pro
Copy link

@2b3pro 2b3pro commented Sep 2, 2025

What this Pull Request (PR) does

Introduce a new MaxTokens flag and configuration option to allow users to specify the maximum number of tokens to generate in AI model responses.

This option is integrated across:

  • Anthropic: Uses MaxTokens for MessageNewParams.
  • Gemini: Sets MaxOutputTokens in GenerateContentConfig.
  • Ollama: Sets num_predict option in chat requests.
  • Dryrun: Includes MaxTokens in the formatted output.

Update example configuration to include maxTokens with a descriptive comment.

@2b3pro
Copy link
Author

2b3pro commented Sep 2, 2025

@ksylvan When I go run ./cmd/generate_changelog --ai-summarize --incoming-pr 1747 I get…

Error: PR validation failed: failed to fetch PR 1747: failed to get PR 1747: GET https://api.github.com/repos/2b3pro/fabric/pulls/1747: 404 Not Found []

Am I doing something wrong?

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 2, 2025

Are you up to date with the latest sources? I fixed this exact problem recently.

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 2, 2025

I looked at your fork on github and it looks like it's trying to run github actions (which probably won't work and may mess up other things). My fork is set up with this setting:

image

This is what I recommend. If your fork is up to date with main then generate_changelog should be looking at the parent fork for where to see (and summarize) the PRs.

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 2, 2025

I'll look into this more carefully tomorrow, while I'm at work, Ian. @2b3pro

@2b3pro
Copy link
Author

2b3pro commented Sep 2, 2025

I'll look into this more carefully tomorrow, while I'm at work, Ian. @2b3pro

I believe, I am up to date on main. Thank you!

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 3, 2025

I believe, I am up to date on main. Thank you!

Can you show me the output of git remote -v and git branch in your clone of Fabric?

@2b3pro
Copy link
Author

2b3pro commented Sep 3, 2025

git remove -v:

origin	https://github.com/2b3pro/fabric.git (fetch)
origin	https://github.com/2b3pro/fabric.git (push)

git branch:

  feature/structured_outputs
  feature/structured_outputs_plugin_architecture
  main
* maxTokens
  schema_plugin

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 3, 2025

Huh... I think I know what's happening. You don't have the upstream parent set up.

Do this instead (using the "gh" cli tool):

~/src $ gh repo clone ksylvan/fabric kayvan-fork  

Cloning into 'kayvan-fork'...
remote: Enumerating objects: 19649, done.
remote: Counting objects: 100% (242/242), done.
remote: Compressing objects: 100% (122/122), done.
remote: Total 19649 (delta 153), reused 120 (delta 120), pack-reused 19407 (from 3)
Receiving objects: 100% (19649/19649), 212.05 MiB | 10.14 MiB/s, done.
Resolving deltas: 100% (10027/10027), done.
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 6 (delta 2), reused 6 (delta 2), pack-reused 0 (from 0)
Unpacking objects: 100% (6/6), 18.01 KiB | 3.60 MiB/s, done.
From github.com:danielmiessler/Fabric
 * [new branch]        main       -> upstream/main
! Repository danielmiessler/Fabric set as the default repository. To learn more about the default repository, run: gh repo set-default --help

And now go into your cloned fork directory:

 ~/src $ cd kayvan-fork 

src/kayvan-fork [main] $ gh pr list

Showing 2 of 2 open pull requests in danielmiessler/Fabric

ID     TITLE                                                       BRANCH                CREATED AT     
#1748  feat: Implement structured output with JSON schema support  2b3pro:schema_plugin  about 1 day ago
#1747  feat: Add MaxTokens option for AI model output control      2b3pro:maxTokens      about 1 day ago

Now checkout the branch:

src/kayvan-fork [main] $ gh pr checkout 1747
remote: Enumerating objects: 31, done.
remote: Counting objects: 100% (31/31), done.
remote: Compressing objects: 100% (14/14), done.
remote: Total 16 (delta 12), reused 5 (delta 1), pack-reused 0 (from 0)
Unpacking objects: 100% (16/16), 1.97 KiB | 201.00 KiB/s, done.
From github.com:danielmiessler/Fabric
 * [new ref]           refs/pull/1747/head -> maxTokens
Switched to branch 'maxTokens'

Let's verify we are in the branch:

src/kayvan-fork [maxTokens] $ git branch
  main
* maxTokens

And now we run the change log tool to generate the snippet:

src/kayvan-fork [maxTokens] $ go run ./cmd/generate_changelog --ai-summarize --incoming-pr 1747 
Commit created successfully. Please review and push manually.
Successfully created incoming changelog entry: cmd/generate_changelog/incoming/1747.txt

Now, let's see what happened with the commit log:

src/kayvan-fork [maxTokens] $ git log --oneline | head -3
e525582b chore: incoming 1747 changelog entry
c9dda9c9 feat: Add MaxTokens option for AI model output control
70f8c013 chore(release): Update version to v1.4.307

And let's look at the generated file:

src/kayvan-fork [maxTokens] $ cat cmd/generate_changelog/incoming/1747.txt
                     
### PR [#1747](https://github.com/danielmiessler/Fabric/pull/1747) by [2b3pro](https://github.com/2b3pro): feat: Add MaxTokens option for AI model output control

- Added MaxTokens configuration option to control the maximum number of tokens generated in AI model responses
- Integrated MaxTokens support across multiple AI providers including Anthropic, Gemini, Ollama, and Dryrun
- Updated example configuration files to include maxTokens parameter with descriptive comments
- Enhanced AI model output control by allowing users to specify token limits for more predictable response lengths
- Implemented provider-specific token limiting using native API parameters (MaxTokens, MaxOutputTokens, num_predict)

And you can "git push" (or rebase into one commit and push) now.

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 3, 2025

I added an issue for myself to improve the CONTRIBUTING document.

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 4, 2025

I see you got it working - Thanks for the bug report and the coding contribution, Ian. @2b3pro

@2b3pro
Copy link
Author

2b3pro commented Sep 4, 2025

I see you got it working - Thanks for the bug report and the coding contribution, Ian. @2b3pro

Actually no. I did what you said above but got an error when I tried to git push. So I just copied the file it generated and put it into my repo.

Thank you for your patience and assistance. You're most appreciated.

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 4, 2025

Oh right... The initial gh clone should be your fork, not mine.

gh repo clone 2b3pro/fabric
cd fabric

Then follow the rest of the instructions.

@2b3pro
Copy link
Author

2b3pro commented Sep 4, 2025

Ok done!

@2b3pro 2b3pro marked this pull request as ready for review September 6, 2025 01:12
@ksylvan
Copy link
Collaborator

ksylvan commented Sep 6, 2025

Tests are failing @2b3pro

@2b3pro 2b3pro marked this pull request as draft September 6, 2025 17:17
@2b3pro 2b3pro marked this pull request as ready for review September 7, 2025 13:17
@2b3pro
Copy link
Author

2b3pro commented Sep 7, 2025

Thanks for your patience, @ksylvan!

@ksylvan
Copy link
Collaborator

ksylvan commented Sep 21, 2025

@2b3pro Are you going to put forward another PR for this? If so, please add the internationalization strings in the locale files too.

@2b3pro 2b3pro reopened this Sep 21, 2025
2b3pro added a commit to 2b3pro/fabric that referenced this pull request Sep 21, 2025
- Add --max-tokens flag to control maximum token output
- Support max_completion_tokens for OpenAI GPT-5 models
- Update all AI providers (Anthropic, OpenAI, Gemini, Ollama, DryRun)
- Add MaxTokens configuration to example.yaml
- Update help documentation and translations
- Add changelog entry for feature danielmiessler#1747
@2b3pro
Copy link
Author

2b3pro commented Sep 21, 2025

@ksylvan Sure. I updated for gpt-5 which apparently changed how it manages max_tokens.

2b3pro added a commit to 2b3pro/fabric that referenced this pull request Sep 21, 2025
- Add --max-tokens flag to control maximum token output
- Support max_completion_tokens for OpenAI GPT-5 models
- Update all AI providers (Anthropic, OpenAI, Gemini, Ollama, DryRun)
- Add MaxTokens configuration to example.yaml
- Update help documentation and translations
- Add changelog entry for feature danielmiessler#1747
2b3pro added a commit to 2b3pro/fabric that referenced this pull request Sep 21, 2025
- Add --max-tokens flag to control maximum token output
- Support max_completion_tokens for OpenAI GPT-5 models
- Update all AI providers (Anthropic, OpenAI, Gemini, Ollama, DryRun)
- Add MaxTokens configuration to example.yaml
- Update help documentation and translations
- Add changelog entry for feature [danielmiessler#1747](https://github.com/2b3pro/fabric/issues/1747)
Copy link
Collaborator

@ksylvan ksylvan left a comment

Choose a reason for hiding this comment

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

Thank you for adding the localization support, Ian.

- Add MaxTokens configuration option allowing users to specify the maximum number of tokens to generate in AI model responses
- Integrate MaxTokens support across multiple AI providers including Anthropic, Gemini, and Ollama with updated CLI flags and example configuration
- Enhance ParseFileChanges function to support both JSON format and markdown format for better compatibility with different AI model outputs
- Support max_completion_tokens for GPT-5 models with conditional logic to map MaxTokens to the appropriate parameter for OpenAI API requests
- Add test case to validate proper parameter mapping for GPT-5 models according to their specific API requirements
- Add internationalization support
@ksylvan
Copy link
Collaborator

ksylvan commented Sep 29, 2025

@2b3pro Can you see what it would take to also add the max tokens parameter to LM Studio?

@2b3pro
Copy link
Author

2b3pro commented Sep 29, 2025

@2b3pro Can you see what it would take to also add the max tokens parameter to LM Studio?

@ksylvan I'll take a look

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what this has to do with the maxTokens change or why this PR is adding 800+ lines to the code base.

This seems like some leftover changes from something else, Ian @2b3pro

Copy link
Author

Choose a reason for hiding this comment

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

Looks like that's from having synced with the more recent versions of fabric

@2b3pro 2b3pro marked this pull request as draft October 5, 2025 14:53
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