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

(docs) Add declarative yaml explanations #129

Merged
merged 7 commits into from
Oct 16, 2024
Merged

(docs) Add declarative yaml explanations #129

merged 7 commits into from
Oct 16, 2024

Conversation

maxmynter
Copy link
Collaborator

No description provided.

@AdrianoKF AdrianoKF added the documentation Improvements or additions to documentation label Oct 9, 2024
@AdrianoKF AdrianoKF linked an issue Oct 10, 2024 that may be closed by this pull request
docs/guide/declarative_containers.md Outdated Show resolved Hide resolved
docs/guide/declarative_containers.md Outdated Show resolved Hide resolved
Comment on lines 3 to 4
jobq allows you to define your Docker environment in a declarative YAML format. Instead of writing a Dockerfile directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Docker environment" leads me in a different direction that what we're trying to achieve: define the contents and requirements for a Docker image build (the environment for me would be the Docker daemon and other stuff that happens on the host machine)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tbh, I struggled writing that because someone who has working associations to all that would likely not use our .yaml format but rather supply a Dockerfile directly. So I was trying to aim the explanations at someone who has but a basic familiarity with containerization.

I do agree though that overloading of vocabulary is bad as we make thorough understanding harder for newbies down the road.

I would, however, avoid diving into the daemon etc. within the docs.


## YAML Structure

The YAML has the following structure.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leading with an example is a good idea! I would then structure the rest of the page by the individual fields that the user can set (so we get a usable page sidenav) and mention which ones are mandatory.

Additionally, I would add a link to the official Dockerfile reference (https://docs.docker.com/reference/dockerfile/), to aid in understanding the meaning of the different options.

Since we currently mirror the structure of a "regular" Dockerfile, with abstractions only in the form of the dependencies map, understanding Dockerfiles is still somewhat required knowledge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree.

Though, if we only have that minor of an abstraction around dockerfiles themselves and we still require underlying docker knowledge, we could think about scrapping .yaml support entirely (or extending it such that it actually hides complexity).

Because, as of now, i don't see that much upside in the .yaml themselves. Dockerfiles are also version controlled.
Or am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that is kind of what I wanted to probe with this PR: see how much (useful) abstraction over Dockerfiles we currently provide.

There is some (dependency management), but a lot of the underlying piping is still shining through.

These insights can guide whether we want to elaborate on the image building at some point or ditch it in favor of the user having to write their own Dockerfiles again.

(I really like the idea of the feature, but at the moment it really is stuck in an uncanny valley of sorts)

base_image: python:3.12-slim
```

Then, you can define system level and Python dependencies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Explain in detail the types of dependencies that can be defined:

  • regular package dependencies
  • requirements files (prefixed with -r, --requirement)
  • Wheel files in the build context (anything ending in .whl)
  • (editable) local installs (path to the project folder, must contain a pyproject.toml; optionally with -e)

In general, it might be helpful to link to the pip docs, where a lot more detail can be found on the requirement specification.

As for apt dependencies, we should let the user know that using those requires a Debian/Ubuntu-derived base image (e.g., the official python images).

docs/guide/declarative_containers.md Outdated Show resolved Hide resolved
docs/guide/declarative_containers.md Outdated Show resolved Hide resolved
docs/guide/declarative_containers.md Outdated Show resolved Hide resolved
- test: test
```

To use the environment specification, you have to add it in your `@job` decorator as a `spec`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reword "environment specification"

Comment on lines 101 to 102
job = Job(
func=your_job_function,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we refer to the @job decorator above, we should not instantiate the Job class manually (additionally overwriting the job name).

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 82.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 56.60%. Comparing base (4b383b3) to head (8b19bf6).
Report is 59 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
client/src/jobq/assembler/config.py 79.26% 17 Missing ⚠️
client/src/jobq/assembler/renderers.py 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   56.42%   56.60%   +0.18%     
==========================================
  Files          61       62       +1     
  Lines        2997     3132     +135     
==========================================
+ Hits         1691     1773      +82     
- Misses       1306     1359      +53     
Flag Coverage Δ
backend 88.27% <ø> (ø)
client 48.62% <82.00%> (+0.69%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AdrianoKF
Copy link
Collaborator

I have pushed a rather large refactoring in d98d738:

  • Changes all the dataclasses into Pydantic models
  • Changes the labels/build args/env mappings into simple list[str], with field validators to ensure they are well-formed
  • Type stopsignal as a constraint int or string, since both are permissible in the Dockerfile syntax as well

This addresses most of my review comments from above but breaks our example files.
If we decide to keep these changes, we will have to adapt those files.

Additionally, we might consider allowing both list and map formats for env/labels/build args to mirror the Docker Compose syntax:

env:
  FOO: val
  BAR: val

# or

env:
  - FOO=val
  - BAR=val

# But not, as we had before

env:
  - FOO: val  # (!)

@maxmynter
Copy link
Collaborator Author

I have one open issue of which I haven't yet found the cause.

When I submit the example-hello.py job it builds successfully but then fails because the docker dependency is missing even though that is only used for type checking and guarded by typing.TYPE_CHECKING.

So we need to figure out why the execution on the server attempts type checking for that example.

cc: @AdrianoKF

@AdrianoKF
Copy link
Collaborator

I have one open issue of which I haven't yet found the cause.

I wasn't able to reproduce that error, but another one: in the client's pyproject.toml, the jobs_execute script was still pointing to the jobs.execute (not jobq), which prevented the container from finding the script.

Fixed it in e6c696d, now the example works fine on my machine.

@maxmynter
Copy link
Collaborator Author

maxmynter commented Oct 16, 2024

This fix also resolved the typing issue mentioned above, even though i don't fully understand how were related.

I'll squash all changes to the docs and then rebase such that we have history of the features/fixes in our main commit log.

maxmynter and others added 7 commits October 16, 2024 14:59
The volumes are only made mountable in the Dockerfile, there is no mounting taking place.

Therefore, a source:target mapping is moot. Even though a renderer was missing in the first place and now added in this commit.
The script was still using the `jobs.execute` module (old spelling).
@maxmynter maxmynter merged commit abdc993 into main Oct 16, 2024
4 checks passed
@maxmynter maxmynter deleted the docs-yaml branch October 16, 2024 13:10
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document declarative image builds
2 participants