Skip to content

Conversation

Json-Andriopoulos
Copy link
Contributor

@Json-Andriopoulos Json-Andriopoulos commented Sep 29, 2025

Describe changes

I implemented/fixed _ to achieve _.

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.
  • I have based my new branch on develop and the open PR is targeting develop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.
  • IMPORTANT: I made sure that my changes are reflected properly in the following resources:
    • ZenML Docs
    • Dashboard: Needs to be communicated to the frontend team.
    • Templates: Might need adjustments (that are not reflected in the template tests) in case of non-breaking changes and deprecations.
    • Projects: Depending on the version dependencies, different projects might get affected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@github-actions github-actions bot added the enhancement New feature or request label Sep 29, 2025
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4005-docker-settings-usage-warnings branch 2 times, most recently from 535f94f to 617ea24 Compare September 30, 2025 08:26
@strickvl
Copy link
Contributor

It's a somewhat common thing to switch between a local orchestrator and a cloud-based one, esp during development. I'm wondering if this warning might get a bit annoying?

@htahir1
Copy link
Contributor

htahir1 commented Sep 30, 2025

100% i wouldnt raise usage warnings for this due to what alex said

@bcdurak
Copy link
Contributor

bcdurak commented Sep 30, 2025

@strickvl @htahir1, I think I disagree with this a little bit. If you are working with a pipeline where you define some DockerSettings and the orchestrator is currently ignoring it, I would want to see the warning.

@bcdurak
Copy link
Contributor

bcdurak commented Sep 30, 2025

It is an indicator of how to do it properly. If I am in the development setting, I would not be bothered by this if I switch to a local orchestrator.

@bcdurak bcdurak linked an issue Oct 1, 2025 that may be closed by this pull request
1 task
@zenml-io zenml-io deleted a comment from github-actions bot Oct 1, 2025
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4005-docker-settings-usage-warnings branch 2 times, most recently from e32d1b6 to c4c25e8 Compare October 1, 2025 09:28
- Warn when used without containerized orchestrator in place
@Json-Andriopoulos Json-Andriopoulos force-pushed the feature/4005-docker-settings-usage-warnings branch from c4c25e8 to 7295416 Compare October 1, 2025 12:21
@htahir1
Copy link
Contributor

htahir1 commented Oct 1, 2025

I think we have talked a few times how overloading warnings for use-cases that dont matter causes people to just ignore them @bcdurak ... I will guarantee this will cauase one of two things a) Most people will ignore this warning b) People will start having if conditions in their code to not apply dockersettings on certain stacks, which is ugly AF

I am quite strongly opposed to this PR tbh

@bcdurak bcdurak requested a review from schustmi October 2, 2025 07:17
@schustmi
Copy link
Contributor

schustmi commented Oct 6, 2025

@htahir1 @Json-Andriopoulos "a) Most people will ignore this warning": This is not an argument against having a warning at all. I think ZenML should warn users about stuff that is worthy of a warning. If they decide to ignore them and run into issues because of it, that's on them and warning them is the best we could do.

Regarding what we should do in this case:

  • When running a pipeline with an "invalid settings key" (e.g. settings={"not_docker": DockerSettings(...)} we currently log an info message:
    logger.info(
    "Not including stack component settings with key `%s`.",
    key,
    )
    This case includes when you specify settings for a component or flavor that is not in your active stack when running.
  • When providing some keys for a settings class which don't exist (e.g. (e.g. settings={"docker": DockerSettings(invalid_key=...)} we currently log a warning message:
    logger.warning(
    "Ignoring invalid setting attributes `%s` defined for key `%s`.",
    list(settings_instance.model_extra),
    key,
    )

The case we're trying to catch as part of this PR is probably more similar to the first bullet point, so maybe changing it to an info log would be the most inline with how we treat it currently? But in my opinion, both the first bullet point here and the message as part of this PR logically are warning messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a warning when no DockerSettings are in use

5 participants