Skip to content

server: eagerly create prompts log directory at startup#25322

Merged
pwilkin merged 1 commit into
ggml-org:masterfrom
rankaiyx:feat/log-prompts-dir-auto-create
Jul 8, 2026
Merged

server: eagerly create prompts log directory at startup#25322
pwilkin merged 1 commit into
ggml-org:masterfrom
rankaiyx:feat/log-prompts-dir-auto-create

Conversation

@rankaiyx

@rankaiyx rankaiyx commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Overview

--log-prompts-dir is used to log prompts during debugging, but it requires the directory to exist at the time of the request.

If the directory is missing, every request triggers an std::ofstream open failure and an SRV_ERR log entry; this not only clutters the runtime logs but also renders the debugging feature unusable.

Create the prompts log directory after parameter parsing instead of relying on it existing at request time.

Uses std::filesystem::create_directories for correct recursive creation on all platforms.

The primary use case is for temporary file systems like /tmp/foo, to avoid having to manually create the directory every time the server reboots.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES . Collaborated with an agent based on Qwen3.6-27B to create the patch. Human reviewed and successfully tested.

@rankaiyx rankaiyx requested a review from a team as a code owner July 5, 2026 14:50
@github-actions github-actions Bot added the server label Jul 5, 2026
@ngxson

ngxson commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

IMO this should better be inside arg.cpp

@rankaiyx rankaiyx force-pushed the feat/log-prompts-dir-auto-create branch from 0e0773a to 4cfebfd Compare July 7, 2026 00:45
@rankaiyx rankaiyx requested review from a team and ngxson as code owners July 7, 2026 00:45
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jul 7, 2026
@rankaiyx

rankaiyx commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

IMO this should better be inside arg.cpp

Changed as suggested.
Also updated the docs.
Removed the CLI docs because that feature isn’t actually implemented.

After testing, it is working properly as expected.

Comment thread common/arg.cpp Outdated
}
}
).set_examples({LLAMA_EXAMPLE_SERVER, LLAMA_EXAMPLE_CLI}));
).set_examples({LLAMA_EXAMPLE_SERVER}));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why did you remove the CLI?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The CLI does not actually implement this functionality; the configuration item is only referenced on the server side.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moreover, the primary use case for this feature does indeed appear to be server-side.
If there is a need for a CLI in the future, it can be added back during implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I want to adhere to the principle of minimal changes, I could also undo the second commit. what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will be soon irrelevant anyway with #24948

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please undo any changes not mentioned in the description

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, that's done.

@rankaiyx rankaiyx force-pushed the feat/log-prompts-dir-auto-create branch from 4cfebfd to 1c7cffe Compare July 7, 2026 14:21
@rankaiyx rankaiyx requested a review from ngxson July 7, 2026 14:53
ngxson
ngxson previously approved these changes Jul 7, 2026
@pwilkin

pwilkin commented Jul 7, 2026

Copy link
Copy Markdown
Member

Nooooo we let a PR with an EditorConfig checker error through, CISC is gonna kill us...

@pwilkin

pwilkin commented Jul 7, 2026

Copy link
Copy Markdown
Member

@rankaiyx could you maybe kill that single EditorConfig error as well?

@ServeurpersoCom

Copy link
Copy Markdown
Contributor
diff --git a/common/speculative.cpp b/common/speculative.cpp
index cfe4130e5c..955ea43052 100644
--- a/common/speculative.cpp
+++ b/common/speculative.cpp
@@ -2311,7 +2311,7 @@ common_init_speculative_result::common_init_speculative_result(
     } else if (spec_mtp) {
         model_path = params.model.path;

-        LOG_TRC("%s: creating MTP draft context against the target model '%s'\n", <- It's a EOL whitespace here
+        LOG_TRC("%s: creating MTP draft context against the target model '%s'\n",
             __func__, model_path.c_str());

         llama_context * ctx_dft = llama_init_from_model(model_tgt, cparams);

@ngxson ngxson dismissed their stale review July 7, 2026 18:39

CI fails

@rankaiyx rankaiyx force-pushed the feat/log-prompts-dir-auto-create branch from 1c7cffe to 7ec6974 Compare July 7, 2026 21:53
@rankaiyx

rankaiyx commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

CI Error Cause: The editorconfig check failed due to trailing whitespace at common/speculative.cpp:2304. This whitespace was already present in upstream/master (introduced by commit f5525f7), not by this PR. It was later fixed by commit c198af4 (#25410), but the CI ran against an older version of master that still contained the issue.

Fix: Rebased this branch onto the latest upstream/master, which already includes the whitespace fix. The CI should now pass.

Let's wait for the CI to turn green.

@pwilkin pwilkin merged commit f296fdf into ggml-org:master Jul 8, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants