Skip to content
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

FR: set QUARTO_DOCUMENT_FILE to the path of the input file #12271

Merged
merged 2 commits into from
Mar 28, 2025

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Mar 12, 2025

This allows execution engine to access it

Related to discussion in #11606

And add test for each engine we support

While adding tests, I noticed two things:

  • options.projectDir is always set now since singleFileProjectContext() so we could simplify codebase maybe (see comment in code)
  • However, QUARTO_DOCUMENT_PATH does not output the same (using the absolute path for project and using a relative path for non-project)

Example

  • Single file project
QUARTO_PROJECT_ROOT: C:\Users\chris\Documents\DEV_R\quarto-cli\tests\docs\smoke-all\jupyter
QUARTO_DOCUMENT_PATH: docs\smoke-all\jupyter
QUARTO_DOCUMENT_FILE: execution-env-var.qmd
  • in Project
QUARTO_PROJECT_ROOT: C:\Users\chris\Documents\DEV_R\quarto-cli\tests\docs\smoke-all\jupyter\execution-env-project
QUARTO_DOCUMENT_PATH: C:\Users\chris\Documents\DEV_R\quarto-cli\tests\docs\smoke-all\jupyter\execution-env-project
QUARTO_DOCUMENT_FILE: execution-env-var.qmd

I don't know if this is expected. Would be a breaking change to fix it for anyone already using this, but it feels like something that should output the same since aim is to consider everything as a project. User shouldn't have to know about singleFile vs project context.

@cderv cderv force-pushed the execution-engine-env/add-filename branch 2 times, most recently from 7731a84 to 7ed6a03 Compare March 12, 2025 15:49
@cscheid
Copy link
Collaborator

cscheid commented Mar 13, 2025

I like this, but we need to do a different implementation.

As we move towards parallelism, we will need to stop depending on the current environment for things that will change depending on the document. Instead, we should change the code to make it so that enough information exists at the point of spawning a subprocess call to make the environment of each subprocess different.

I can explain more in our engineering meetings, but I don't think we should merge this PR as it stands.

@cderv
Copy link
Collaborator Author

cderv commented Mar 13, 2025

Always two steps ahead, thinking into the future! 😄

I was trying to add this as simply as possible for an easy new feature in the current state of the code. As it is useful information in addition to the existing environment variable.

And me thinking it was an easy addition 🤦

QUARTO_DOCUMENT_PATH would also change depending on the document, right? If so, then adding QUARTO_DOCUMENT_FILE would just add one other en var to handle transition for parallelism 😅 🙄

😄 happy to put that on hold anyhow

@cderv cderv added this to the Future milestone Mar 13, 2025
@vccortez
Copy link

I was about to code exactly this, @cderv, coming from that issue, as it seemed simple enough.

regarding the parallelism concerns, is the current version already doing this subprocess spawning ?

@cderv cderv marked this pull request as draft March 21, 2025 16:34
@cderv
Copy link
Collaborator Author

cderv commented Mar 21, 2025

Regarding the parallelism concerns, is the current version already doing this subprocess spawning?

There are some async happening, so it may be already not working as expected in some context. Though this is indeed simple enough, and we could live with this addition until we would change the all logic of where to set those env var.

However, we still have an issue to understand right now before merging this because depending on context (singleFile or project) the content is either relative or absolute.

So I am putting on hold right to also fix this.

@cderv
Copy link
Collaborator Author

cderv commented Mar 24, 2025

However, QUARTO_DOCUMENT_PATH does not output the same (using the absolute path for project and using a relative path for non-project)

@cscheid I have looked into the difference between project render and singlefile render and the rev

Both start with render() which makes the input relative to Deno.cwd()

renderResultInput = relative(Deno.cwd(), walk.path) || ".";
if (renderResult) {
renderResult.context.cleanup();
}
renderResult = await render(renderResultInput, {

However, when in a project, there will be some context compute which will end up normalizing (by adding back the Deno.cwd() like the following in renderProjects

let projectRenderConfig = await computeProjectRenderConfig({
context,
projType,
projOutputDir,
projDir,
options: pOptions,
files: pFiles,
});

// file normaliation
const normalizeFiles = (targetFiles: string[]) => {
return targetFiles.map((file) => {
const target = isAbsolute(file) ? file : join(Deno.cwd(), file);
if (!existsSync(target)) {
throw new Error("Render target does not exist: " + file);
}
return normalizePath(target);
});
};
if (inputs.files) {
if (alwaysExecuteFiles) {
alwaysExecuteFiles = normalizeFiles(alwaysExecuteFiles);
inputs.files = normalizeFiles(inputs.files);
} else if (inputs.options.useFreezer) {
inputs.files = normalizeFiles(inputs.files);
}
}

So basically,

  • When this is a project, renderFiles is called with an absolute path

    const fileResults = await renderFiles(
    projectRenderConfig.filesToRender,

  • When not inside a project, so singleFile render, renderFiles() is called with a relative path

    // otherwise it's just a file render
    const result = await renderFiles(
    [{ path }],
    options,
    nbContext,
    undefined,
    undefined,
    context,
    );

This is the different we see in those environment variable from this PR, as it will be inherited by target.source

Deno.env.set("QUARTO_DOCUMENT_PATH", dirname(options.target.source));

It seems there is room for improvement to make things work the same. But do we want to do it in this PR?

We could

  • Just add QUARTO_DOCUMENT_FILE in the current status as the basename(), complementary to QUARTO_DOCUMENT_PATH`, and deal with the absolute vs relative on its own

  • Add QUARTO_DOCUMENT_FILE but "patch" the output by making the relative be exposed as absolute i.e. Deno.env.set("QUARTO_DOCUMENT_PATH", join(Deno.cwd(), dirname(options.target.source)));

  • Don't add QUARTO_DOCUMENT_FILE until everything is fixed and cleaner.

@cderv cderv force-pushed the execution-engine-env/add-filename branch from 7ed6a03 to a88e7cc Compare March 24, 2025 13:14
@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 28, 2025
cderv added 2 commits March 28, 2025 15:14
…e so that execution engine can access it

And add test for each engine we support
@cderv cderv force-pushed the execution-engine-env/add-filename branch from a88e7cc to d2cd890 Compare March 28, 2025 14:16
@cderv cderv marked this pull request as ready for review March 28, 2025 14:17
@cderv cderv removed the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 28, 2025
@cderv cderv merged commit fd3aacf into main Mar 28, 2025
49 checks passed
@cderv cderv deleted the execution-engine-env/add-filename branch March 28, 2025 14:46
cderv added a commit that referenced this pull request Mar 28, 2025
cderv added a commit that referenced this pull request Mar 28, 2025
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.

3 participants