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

feat: conditionally run pipelines in debug mode #223

Merged

Conversation

YolanFery
Copy link
Contributor

@YolanFery YolanFery commented Dec 3, 2024

We want to provide a way for users to decide if they want to generate debug messages for the pipeline run. By default they will not be stored and shown.

Changes

  • Create and document a Dockerfile to create an image to easily test the local changes on a pipeline run
  • Get the log_level settings from the environment and decide based on that if logs should be stored
  • For Ruff, ignore missing docstrings in tests, I don't think they are required
  • Unit test to filter the logs based on the settings

How/what to test

  • You can test changes in the SDK repo using the docker image created using the Readme instructions
  • Logs messages are correctly filtered based on the settings

Screenshots / screencast

screen-capture.3.webm

Related PRs

Comment on lines -103 to -104
if priority not in valid_priorities:
raise ValueError(f"priority must be one of {', '.join(valid_priorities)}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not need to check with the new Typing

@@ -50,7 +50,7 @@ def download_pipeline(url: str, token: str, run_id: str, target_dir: str):
pipelineRun(id: $id) {
id
version {
number
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue also solved in #221 but not yet merged

@@ -67,6 +67,7 @@ include-package-data = true
[tool.ruff]
line-length = 120
ignore = ["E501"]
per-file-ignores = { "tests/**/test_*.py" = ["D100","D101","D102", "D103"] } # Ignore missing docstrings in tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A preference but I am ok spending more time to provide some docstrings if we think it's valuable

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok for the tests.

Copy link
Member

@qgerome qgerome left a comment

Choose a reason for hiding this comment

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

Looks fine for me. The Priority word is weird to me and would intuitively call that "LogLevel" but if you are not disturbed by this, I can live with it.

@@ -67,6 +67,7 @@ include-package-data = true
[tool.ruff]
line-length = 120
ignore = ["E501"]
per-file-ignores = { "tests/**/test_*.py" = ["D100","D101","D102", "D103"] } # Ignore missing docstrings in tests
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok for the tests.

@YolanFery
Copy link
Contributor Author

Looks fine for me. The Priority word is weird to me and would intuitively call that "LogLevel" but if you are not disturbed by this, I can live with it.

I agree it looks odd but that's what we used in the GraphQL layer and it is also stored in the db ([{"message": "Current dir /home/jovyan", "**priority**": "INFO", "timestamp": "2024-12-02T15:00:11.094187"}, {"message": "In task 1...", "**priority**": "INFO", "timestamp": "2024-12-02T15:00:12.995440"}, {"message": "In task 2... count is 42", "**priority**": "INFO", "timestamp": "2024-12-02T15:00:13.469673"}])

So I decided to avoid complex renames and keep the same name.

I can :

  1. Invest more to rename it everywhere and do the migration of existing elements in the database
  2. Keep it like this
  3. Rename the Enum and leave the GraphQL layer unchanged

Option 3 might be a good one, but let's agree @qgerome

@qgerome
Copy link
Member

qgerome commented Dec 3, 2024

Looks fine for me. The Priority word is weird to me and would intuitively call that "LogLevel" but if you are not disturbed by this, I can live with it.

I agree it looks odd but that's what we used in the GraphQL layer and it is also stored in the db ([{"message": "Current dir /home/jovyan", "**priority**": "INFO", "timestamp": "2024-12-02T15:00:11.094187"}, {"message": "In task 1...", "**priority**": "INFO", "timestamp": "2024-12-02T15:00:12.995440"}, {"message": "In task 2... count is 42", "**priority**": "INFO", "timestamp": "2024-12-02T15:00:13.469673"}])

So I decided to avoid complex renames and keep the same name.

I can :

  1. Invest more to rename it everywhere and do the migration of existing elements in the database
  2. Keep it like this
  3. Rename the Enum and leave the GraphQL layer unchanged

Option 3 might be a good one, but let's agree @qgerome

Let's go for option 3 then :)

@YolanFery YolanFery requested a review from qgerome December 3, 2024 16:29
@qgerome qgerome merged commit 6f495f7 into main Dec 4, 2024
4 checks passed
@qgerome qgerome deleted the HEXA-1076-implement-a-way-to-run-pipelines-in-debug-mode branch December 4, 2024 08: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