-
Notifications
You must be signed in to change notification settings - Fork 551
Feature:4005 Docker settings usage warnings #4011
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
Feature:4005 Docker settings usage warnings #4011
Conversation
535f94f to
617ea24
Compare
|
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? |
|
100% i wouldnt raise usage warnings for this due to what alex said |
|
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. |
c4c25e8 to
7295416
Compare
|
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 |
|
@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:
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. |
|
Overall: the intent (alerting when
This balances the concerns in the thread: folks worried about annoyance when switching locally (valid) and those wanting a clear heads-up when a real misconfiguration happens (also valid). I'm happy if at least first three of the above are taken into account |
|
@htahir1 @Json-Andriopoulos IMO we could simply add a warning log message in the compiler class (in the place I linked above) if there are non-empty docker settings defined when running with a local orchestrator? |
7295416 to
875e625
Compare
ZenML CLI Performance Comparison (Threshold: 1.0s, Timeout: 60s, Slow: 5s)❌ Failed Commands on Current Branch (feature/4005-docker-settings-usage-warnings)
🚨 New Failures IntroducedThe following commands fail on your branch but worked on the target branch:
Performance Comparison
Summary
Environment Info
|
f18f13b to
b9e7252
Compare
acc1871 to
db9a427
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the warning registry! We should definitely add a way to ignore certain warning codes, which should be persisted locally and probably also forwarded to the execution environment where pipelines/steps get executed?
src/zenml/utils/warnings/registry.py
Outdated
| ) | ||
|
|
||
|
|
||
| class WarningCodes(str, Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a StrEnum base class that we usually use in those cases, has some additional properties that you might/might not need :)
| """ | ||
| self._log(warning_code, message, LoggingLevels.WARNING, **kwargs) | ||
|
|
||
| def info(self, *, warning_code: str, message: str, **kwargs: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def info(self, *, warning_code: str, message: str, **kwargs: Any) -> None: | |
| def info(self, *, warning_code: WarningCodes, message: str, **kwargs: Any) -> None: |
We can use the enum here, no? Same in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
| def __init__(self) -> None: | ||
| """WarningController constructor.""" | ||
| self._warning_configs: dict[str, WarningConfig] = {} | ||
| self._warning_statistics: dict[str, int] = defaultdict(int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also from collections import Counter which we could use here, I'll leave it up to you.
Has some helper methods to e.g. count the total amount of warnings, in case we ever want to log some statistics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, good idea. I mainly want to avoid constructing values if the key is missing, Counter supports that as well.
| ): | ||
| return | ||
|
|
||
| warning_message = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do actually have a few orchestrators that are not containerized but still use some DockerSettings attributes to configure their behaviour (WheeledOrchestrator, LightningOrchestrator). So we either need to get rid of this warning here or also include those two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Do we have any production orchestrators that don't make use of the docker settings? Seems like we just need to catch the LocalOrchestrator here and info the message 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think just the local one ignores them entirely
8d88486 to
2b5bd20
Compare
a7af632 to
b4e18c7
Compare
b4e18c7 to
f24d34e
Compare
f24d34e to
6c61de7
Compare
- Warn when used without containerized orchestrator in place
6c61de7 to
58a9abd
Compare
Describe changes
I implemented/fixed _ to achieve _.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes