I would lean more towards removing it completely as I do agree having it
suddenly fail in a production env would be weird.
If we want to keep it, what are you thoughts on locking critical
functionality when this parameter is used? That way while you're testing
things out you can use it (idk how much it's reducing effort seeing as you
mentioned writing a docker container on top of ours isn't a significant
overhead) and then anything close to production forces you to do things the
right way?
--
Regards,
Aritra Basu
On Tue, Aug 29, 2023, 6:55 PM Andrey Anshin <andrey.ans...@taragol.is>
wrote:
> > Sending frequent messages is a good idea. I think we can
> also (additionally) make airflow display a warning same as sqlite warning /
> sequential executor).
>
> I think one difference between Sequential Executor and
> _PIP_ADDITIONAL_REQUIREMENTS.
> Sequential Executor is part of Airflow Core, and there is no
> appearance of _PIP_ADDITIONAL_REQUIREMENTS
> in Airflow Core, this only part of docker image.
>
> For implements this warning we need to make Airflow Core know about this
> environment variable, even if it
> os.environ.get("_PIP_ADDITIONAL_REQUIREMENTS",
> "")
>
>
> ----
> Best Wishes
> *Andrey Anshin*
>
>
>
> On Tue, 29 Aug 2023 at 17:04, Jarek Potiuk <ja...@potiuk.com> wrote:
>
> > @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.
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
