@Artira - yes. Sending frequent messages is a good idea. I think we can
also (additionally) make airflow display a warning same as sqlite warning /
sequential executor).
(though I know for a fact a number of users are just conditioned to click
any warnings like that and ignore them). Only after we ask many questions
it turns out they are using Sequential Executor which we explicitly tell
them not to use and display both log and UI warnings about. Even if they
have 100% perfectly viable production-ready LocalExecutor with parallelism
= 1 even if they want to do something "like Sequential Executor".
So I am worried if that will be enough :)
J.
On Tue, Aug 29, 2023 at 2:57 PM Jarek Potiuk <ja...@potiuk.com> wrote:
> Yes. I contemplated removing it completely and we could straight away fail
> an image that has it set and tell the user how to build the image and how
> to use the `--build` flag of docker-compose.
>
> That was my second-best option.
>
> J.
>
>
> On Tue, Aug 29, 2023 at 2:49 PM Maciej Obuchowski <
> obuchowski.mac...@gmail.com> wrote:
>
>> I feel like straight up disallowing the feature, or disabling it in future
>> versions
>> and just strongly logging for now is the better option than failing after
>> 10 minutes.
>>
>> Generally I think introducing "feature" that would work locally and on dev
>> environment without noticeable problems - and failing on production is
>> wrong.
>> If I'm doing something wrong then either don't let me do that or just tell
>> me to not do it.
>>
>> For all the pros, they hold also for removing _PIP_ADDITIONAL_REQUIREMENTS
>> - so what's the rationale for still keeping that?
>>
>> wt., 29 sie 2023 o 14:30 Aritra Basu <aritrabasu1...@gmail.com>
>> napisał(a):
>>
>> > I haven't used this feature much yet, so not sure on the use case but
>> the
>> > 10 minutes seems a bit arbitrary to me considering I do often test out
>> dags
>> > that take longer than 10 mins to run? Would it perhaps make more sense
>> that
>> > if you launch using the variable on any failure it throws an error
>> saying
>> > you used this variable and that might cause this, retry without using
>> this
>> > and see if you still hit the issue? And perhaps send out warnings every
>> X
>> > minutes/seconds saying you're using this feature which is not
>> recommended.
>> > I'm not sure how frequent warnings of that kind might discourage it's
>> usage
>> > in prod?
>> >
>> > --
>> > Regards,
>> > Aritra Basu
>> >
>> > On Tue, Aug 29, 2023, 4:59 PM Andrey Anshin <andrey.ans...@taragol.is>
>> > wrote:
>> >
>> > > Airflow Trial Mode ON :D
>> > >
>> > > > It is aggressive, yes. I wonder what you think. Is it too
>> aggressive?
>> > >
>> > > I think is not enough :D This one would be
>> > >
>> > > if [[ -n "${_PIP_ADDITIONAL_REQUIREMENTS=}" ]] ; then
>> > > echo "Longrid about best practices"
>> > > exit 42
>> > >
>> > > In this case it is very easy to get a quick answer for an
>> > issue/discussion
>> > > like "My Airflow deployment does not start with exit code 42", rather
>> > than
>> > > "My Airflow deployment constantly restarted".
>> > >
>> > > But I'm not sure if it is applicable and user-friendly.
>> > >
>> > > ----
>> > > Best Wishes
>> > > *Andrey Anshin*
>> > >
>> > >
>> > >
>> > > On Tue, 29 Aug 2023 at 15:01, Jarek Potiuk <ja...@potiuk.com> wrote:
>> > >
>> > > > Hello everyone,
>> > > >
>> > > > It happens more and more often (and seems that it is pretty
>> > > > widespread among "small" users of Airflow using Docker Compose and
>> > > official
>> > > > Airflow image that they are using _PIP_ADDITIONAL_REQUIREMENTS for
>> > > > production usage - even if building and hosting your modified image
>> is
>> > as
>> > > > easy as copy & pasting few lines to a Dockerfile and running `docker
>> > > build`
>> > > > following by `docker push.
>> > > >
>> > > > While I understand (and that's why we have it from the very
>> beginning)
>> > > why
>> > > > you would like to use it for local quick iteration and testing,
>> using
>> > it
>> > > > for anything close to production has a lot of negative consequences
>> and
>> > > > leads to mysterious errors that take a lot of time for those users
>> who
>> > > are
>> > > > trying to investigating some dependency problems, slow start of
>> > > containers,
>> > > > race conditions and it makes them prone to security issues.
>> > > >
>> > > > I thought about whether we should remove the feature completely (it
>> is
>> > > > clearly marked as "do not use it for anything in production - so we
>> > > could).
>> > > > But I thought that a better solution could be to make the default
>> image
>> > > of
>> > > > ours simply unusable for production use if you use the flag.
>> > > >
>> > > > So I came up with this proposed PR:
>> > > > https://github.com/apache/airflow/pull/33879
>> > > >
>> > > > It has a few protections against other side effects (such as
>> > accidentally
>> > > > downgrading or upgrading airflow by adding conflicting requirements,
>> > and
>> > > > verification if the dependencies pass `pip check`) - but the gist of
>> > it -
>> > > > the container will shut down automatically after 10 minutes if it
>> has
>> > > been
>> > > > started with _PIP_ADDITIONAL_REQUIREMENTS (and will print
>> instructions
>> > > what
>> > > > to do - how to build your image - to avoid it). And we have much
>> more
>> > > > comprehensive and detailed instructions on how to build your image
>> and
>> > > even
>> > > > how to integrate the "--build" docker-compose workflow to do the
>> same
>> > > > following the way docker-compose was designed.
>> > > >
>> > > > Yes. It is super aggressive. Yes it will break a number of user's
>> > > workflows
>> > > > (especially when they are using 3rd-party charts that encourage
>> using
>> > > > _PIP_ADDITIONAL_REQUIREMENTS) - part of the problem is that some
>> charts
>> > > out
>> > > > there are exposing this as a "production functionality" despite it
>> not
>> > > > being one.
>> > > >
>> > > > But IMHO - using the variable in production does more harm than good
>> > and
>> > > it
>> > > > has very easy alternatives, which users don't use because they
>> choose
>> > > > shortcuts without realising the consequences - which is not the way
>> you
>> > > > should treat a production environment.
>> > > >
>> > > > Also restarting the container after 10 minutes is pretty consistent
>> > with
>> > > > the usage that _PIP_ADDITIONAL_REQUIRENENTS has been added for.
>> > > Restarting
>> > > > the container in a quick test/dependency iteration environment
>> > regularly
>> > > > could even be seen as a "feature" if you approach the "formal"
>> status
>> > of
>> > > > this variable.
>> > > >
>> > > > Also this has a bit of a "sneaky" way of tricking people into
>> > > > "good practices" learning. In order to avoid the restart you really
>> > need
>> > > to
>> > > > .... build your own image ... So this has also an educational
>> aspect -
>> > > > people will have to learn about building the image anyway if they
>> will
>> > > want
>> > > > to learn how to "not build their images" and use something like the
>> > > > variable again and it will be even more complex than just adding the
>> > > > requirements to the custom image - so they will literally have to
>> learn
>> > > the
>> > > > "good practice".
>> > > >
>> > > > It is aggressive, yes. I wonder what you think. Is it too
>> aggressive?
>> > > > Should we do it?
>> > > >
>> > > > Maybe there are really good reasons why it's impossible to use your
>> own
>> > > > custom images other than the extra step and annoyance of being
>> able to
>> > > > push your images to some - public (free) or private (internal)
>> > registry.
>> > > I
>> > > > can't imagine how someone deploying docker-compose or k8s does not
>> have
>> > > the
>> > > > way of having their own images available. Even if it means . that
>> you
>> > > have
>> > > > to go through some kind of infra of yours you generally should do
>> for
>> > > > production.
>> > > >
>> > > > Looking forward to your comments :D
>> > > >
>> > > > J.
>> > > >
>> > >
>> >
>>
>
