You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2024/03/27 21:45:40 UTC
[PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
dstandish opened a new pull request, #38563:
URL: https://github.com/apache/airflow/pull/38563
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1553938903
##########
airflow/cli/commands/task_command.py:
##########
@@ -278,7 +279,15 @@ def _run_task_by_local_task_job(args, ti: TaskInstance | TaskInstancePydantic) -
external_executor_id=_extract_external_executor_id(args),
)
try:
- ret = run_job(job=job_runner.job, execute_callable=job_runner._execute)
+ # If internal API is used, we must pass session=None so that one is not created
+ # by the provide_session decorator. All downstream functions that actually use
+ # a session are already set up for RPC
+ # todo: perhaps instead we should simply modify the provide_session decorator
+ # to *not* provide a session when using internal API
Review Comment:
I'd say provide_session should provide a fake session that should:
1) store traceback where it has been provided for the first time
2) raise an exception when it is accessed for anything explaining both:
a) where it has been provided
b) where it has been used
c) eventually explanation what to do to contributor.
Both (in CI / breeze / development environment) using `rich`'s traceback functionality (https://rich.readthedocs.io/en/stable/traceback.html).
I see this as CI/Development/Diagnostic tool. If we return None, the most we will get is an exception that None has missing property - which willl be difficult to really trace, because the session can be passed throuh multiple layers and multiple method calls before it is used.
When we enable internal API, there will be a test in CI failing if somone will add a new DB call and did not test it with internal API, so what we really want in this case is to provide the fastest path for the contributor to find out what they should do to fix the problem. I don't think passing None is good enough for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #38563:
URL: https://github.com/apache/airflow/pull/38563#issuecomment-2043674071
ok.... renaming this one to `Add session that blows up when using internal API` because, with the addition of this, there's no longer a need to make the other changes I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1553938903
##########
airflow/cli/commands/task_command.py:
##########
@@ -278,7 +279,15 @@ def _run_task_by_local_task_job(args, ti: TaskInstance | TaskInstancePydantic) -
external_executor_id=_extract_external_executor_id(args),
)
try:
- ret = run_job(job=job_runner.job, execute_callable=job_runner._execute)
+ # If internal API is used, we must pass session=None so that one is not created
+ # by the provide_session decorator. All downstream functions that actually use
+ # a session are already set up for RPC
+ # todo: perhaps instead we should simply modify the provide_session decorator
+ # to *not* provide a session when using internal API
Review Comment:
I'd say provide_session should provide a fake session that should:
1) store traceback where it has been used to provide session for the first time
2) raise an exception when it is accessed for anything explaining both:
a) where it has been provided
b) where it has been used
c) eventually explanation what to do to contributor.
Both (in CI / breeze / development environment) using `rich`'s traceback functionality (https://rich.readthedocs.io/en/stable/traceback.html).
I see this as CI/Development/Diagnostic tool. If we return None, the most we will get is an exception that None has missing property - which willl be difficult to really trace, because the session can be passed throuh multiple layers and multiple method calls before it is used.
When we enable internal API, there will be a test in CI failing if somone will add a new DB call and did not test it with internal API, so what we really want in this case is to provide the fastest path for the contributor to find out what they should do to fix the problem. I don't think passing None is good enough for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1552424445
##########
airflow/cli/commands/task_command.py:
##########
@@ -278,7 +279,15 @@ def _run_task_by_local_task_job(args, ti: TaskInstance | TaskInstancePydantic) -
external_executor_id=_extract_external_executor_id(args),
)
try:
- ret = run_job(job=job_runner.job, execute_callable=job_runner._execute)
+ # If internal API is used, we must pass session=None so that one is not created
+ # by the provide_session decorator. All downstream functions that actually use
+ # a session are already set up for RPC
+ # todo: perhaps instead we should simply modify the provide_session decorator
+ # to *not* provide a session when using internal API
Review Comment:
@uranusjr thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add session that blows up when using internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1557679346
##########
airflow/utils/session.py:
##########
@@ -24,12 +24,17 @@
from sqlalchemy.orm import Session as SASession
from airflow import settings
+from airflow.api_internal.internal_api_call import InternalApiConfig
+from airflow.settings import TracebackSession
from airflow.typing_compat import ParamSpec
@contextlib.contextmanager
def create_session() -> Generator[SASession, None, None]:
"""Contextmanager that will create and teardown a session."""
+ if InternalApiConfig.get_use_internal_api():
Review Comment:
Why?
I thought that `database_access_isolation` might be true globally for the deployment but e.g. scheduler and webserver would disregard that and still connect to DB? I.e. with `force_database_direct_access()`?
If that's not true, perhaps we should remove `force_database_direct_access` and just throw an error if scheduler / webserver have `database_access_isolation` True?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1542184733
##########
airflow/cli/commands/task_command.py:
##########
@@ -278,7 +279,15 @@ def _run_task_by_local_task_job(args, ti: TaskInstance | TaskInstancePydantic) -
external_executor_id=_extract_external_executor_id(args),
)
try:
- ret = run_job(job=job_runner.job, execute_callable=job_runner._execute)
+ # If internal API is used, we must pass session=None so that one is not created
+ # by the provide_session decorator. All downstream functions that actually use
+ # a session are already set up for RPC
+ # todo: perhaps instead we should simply modify the provide_session decorator
+ # to *not* provide a session when using internal API
Review Comment:
Thinking the same, the decorator should just be made smarter about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1556179515
##########
airflow/cli/commands/task_command.py:
##########
@@ -278,7 +279,15 @@ def _run_task_by_local_task_job(args, ti: TaskInstance | TaskInstancePydantic) -
external_executor_id=_extract_external_executor_id(args),
)
try:
- ret = run_job(job=job_runner.job, execute_callable=job_runner._execute)
+ # If internal API is used, we must pass session=None so that one is not created
+ # by the provide_session decorator. All downstream functions that actually use
+ # a session are already set up for RPC
+ # todo: perhaps instead we should simply modify the provide_session decorator
+ # to *not* provide a session when using internal API
Review Comment:
i'm working on something like this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add session that blows up when using internal API [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1557175740
##########
airflow/utils/session.py:
##########
@@ -24,12 +24,17 @@
from sqlalchemy.orm import Session as SASession
from airflow import settings
+from airflow.api_internal.internal_api_call import InternalApiConfig
+from airflow.settings import TracebackSession
from airflow.typing_compat import ParamSpec
@contextlib.contextmanager
def create_session() -> Generator[SASession, None, None]:
"""Contextmanager that will create and teardown a session."""
+ if InternalApiConfig.get_use_internal_api():
Review Comment:
should be:
```
if conf.getboolean("core", "database_access_isolation", fallback=False)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make _run_task_by_local_task_job compatible with internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1548185069
##########
airflow/cli/commands/task_command.py:
##########
@@ -278,7 +279,15 @@ def _run_task_by_local_task_job(args, ti: TaskInstance | TaskInstancePydantic) -
external_executor_id=_extract_external_executor_id(args),
)
try:
- ret = run_job(job=job_runner.job, execute_callable=job_runner._execute)
+ # If internal API is used, we must pass session=None so that one is not created
+ # by the provide_session decorator. All downstream functions that actually use
+ # a session are already set up for RPC
+ # todo: perhaps instead we should simply modify the provide_session decorator
+ # to *not* provide a session when using internal API
Review Comment:
so to be clear, you think `@provide_session` should just provide `None` object when internal API configured? @potiuk thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add session that blows up when using internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1557679346
##########
airflow/utils/session.py:
##########
@@ -24,12 +24,17 @@
from sqlalchemy.orm import Session as SASession
from airflow import settings
+from airflow.api_internal.internal_api_call import InternalApiConfig
+from airflow.settings import TracebackSession
from airflow.typing_compat import ParamSpec
@contextlib.contextmanager
def create_session() -> Generator[SASession, None, None]:
"""Contextmanager that will create and teardown a session."""
+ if InternalApiConfig.get_use_internal_api():
Review Comment:
Why?
I thought that `database_access_isolation` might be true globally for the deployment but e.g. scheduler and webserver would disregard that and still connect to DB? I.e. with `force_database_direct_access()`?
If that's not true, perhaps instead of `force_database_direct_access` we should just throw an error if scheduler / webserver have `database_access_isolation` True?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add session that blows up when using internal API [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #38563:
URL: https://github.com/apache/airflow/pull/38563
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add session that blows up when using internal API [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38563:
URL: https://github.com/apache/airflow/pull/38563#discussion_r1557737127
##########
airflow/utils/session.py:
##########
@@ -24,12 +24,17 @@
from sqlalchemy.orm import Session as SASession
from airflow import settings
+from airflow.api_internal.internal_api_call import InternalApiConfig
+from airflow.settings import TracebackSession
from airflow.typing_compat import ParamSpec
@contextlib.contextmanager
def create_session() -> Generator[SASession, None, None]:
"""Contextmanager that will create and teardown a session."""
+ if InternalApiConfig.get_use_internal_api():
Review Comment:
Ah yes. Now I re-read it, it's been a while.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org