Fix comment on workdir (very confusing)

[ekzhang]

Extremely confusing field called workdir on ImageMetadata which is not actually used by any of our systems, ImageManifest.workdir is the actual workdir. This is wrong in 99% of images.

[mwaskom]

Not sure I know the initial intention here or exactly the nature of the confusion, but I think this is meant to convey what the workdir state would be for subsequent builder tasks, (i.e. so that we can know where .add_local_* layers would put their files with a relative remote location. @freider might know more.

[ekzhang]

Yeah there's a somewhat confusing issue in that image.manifest.workdir and image.workdir are different. image.manifest.workdir is the actual workdir, and it's read by tasks on startup:

• runner tasks: uses manifest.workdir, otherwise /root if not set
• builder tasks: uses manifest.workdir, otherwise / if not set
• sandbox tasks: uses sandbox.workdir, otherwise manifest.workdir if not set, otherwise / if not set

Meanwhile image.workdir is not read by anything in the runtime and is a duplicate metadata value that is measured by builder.

The issue is that for the vast majority of tasks, manifest.workdir is not set, but in the internal lookup and database for images, we see workdir: / observed by this metadata process. Which is misleading — it's actually not set, and the runtime code treats it as not set.

[mwaskom]

I'm not sure I'm completely following, but is it right that what's confusing here is that we have a different default workdir for builder tasks vs runner tasks?

[ekzhang]

I'm not sure I'm completely following, but is it right that what's confusing here is that we have a different default workdir for builder tasks vs runner tasks?

That's not quite it, the confusing but is that this field is just useless and not used anywhere. The correct field is manifest.workdir. I can update the comment to make that clearer

[ekzhang]

@prbot approve freider

[modal-pr-review-automation]

Please request a reviewer to follow up with a post-merge review.

[modal-pr-review-automation]

Approved 👍. @freider will follow-up review this.

[ekzhang]

See https://github.com/modal-labs/modal/pull/22955 for comment in other location of the code

[freider]

The context here is that for some planned future work the client needs to receive the image workdir in the response data from images. We need to fill this metadata field with whatever the best information available - be that from the manifest or otherwise.

Also, why is it wrong for 99% of images? (disregarding that we override it for runner tasks)

[ekzhang]

Thanks for the insight @freider, I can update the comment / document better what it means when manifest.workdir is missing. The missing value is actually semantic versus a / value.

I think for now any workdir should return a PurePosixPath | None or str | None value determining whether it is set, since None is meaningful (and it's the case for 99% of images mentioned above) — overriding as /root only applies when None. Does that make sense? Just from a runtime perspective.

Either way I think we shouldn't have two different workdir on the backend -- for client code it's up for discussion! In the backend, the source of truth is the manifest in all cases.