Re: [PR] Remove `internal_api_call` decorator [airflow]
potiuk commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508405075 Then - this one can be rebased gradually until we get to "no change" :D -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove `internal_api_call` decorator [airflow]
potiuk commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508402762 And yes. Cool you are looking at it @shahar1 -> i think a good idea will be by anyone who starts looking at removing the method from one of those files announce "I am taking this part" in #446436 in order to avoid duplication :) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove `internal_api_call` decorator [airflow]
potiuk commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508397402 > In this case, (please @potiuk correct me if I am wrong) I dont think we should re-join this method. _execute_task_callbacks is called in different methods and the separation makes sense. Yes. We can maybe even nicely refactor it to make more sense and rename such methods, place them in the "right" place etc. This should be case-by-case decision and that's why in #44436 I extracted all the method lists so that we can do it in pieces - maybe not one-by-one, but maybe file-by-file, so that we could review the change in a meaningful way. There are, for example, some cases where likely after re-joining we will be able to undo some of the changes made (for example there were few methods where we artifficially had to add commit() where otherwise it was not needed - because the logic was split into "client" and "server" in a sequence of transctions. By having smaller, focused commits "per-file" we can look at the historical changes to it and when we extracted it and decide what to do. Also splitting it "per-group" might be easily distributed between different people in this case. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove `internal_api_call` decorator [airflow]
shahar1 commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508295954 > > Yes, that's why I just drafted it :) > > Cool :) > > > I'll take care of it, but just to make sure that I understand "re-join" correctly - everywhere where there's both def fun() and def _fun(), simply restore the logic from _fun() to fun(), or is it deeper than that? > > Yes you understood well what re-join means. When working on AIP-44, many methods were moved to private functions to make them "callable". You'll see many methods like: > > ``` > @classmethod > @internal_api_call > def function_name(self, ...) > return _function_name(...) > ``` > > The goal is to move back the implementation of `_function_name` into `function_name`. > > > Also, what should I do if _fun() is called more than once like ./airflow/dag_processing/processor.py: _execute_task_callbacks? (it's my first time in the API area) > > In this case, (please @potiuk correct me if I am wrong) I dont think we should re-join this method. `_execute_task_callbacks` is called in different methods and the separation makes sense. > > > What should we do about these functions in rcp_api.py? Rename them to their original? > > Are you talking about `airflow/api_internal/endpoints/rpc_api_endpoint.py`? This file has been removed recently, so no need to take care of it > > > It's my first time in the API area, so I'll try not to break too many things :) > > No worries! Thanks for doing it! We all have to start someday right? I'm talking about `providers/src/airflow/providers/edge/worker_api/routes` (maybe @jscheffl could help with this one :) ) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove `internal_api_call` decorator [airflow]
vincbeck commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508289477 > Yes, that's why I just drafted it :) Cool :) > I'll take care of it, but just to make sure that I understand "re-join" correctly - everywhere where there's both def fun() and def _fun(), simply restore the logic from _fun() to fun(), or is it deeper than that? Yes you understood well what re-join means. When working on AIP-44, many methods were moved to private functions to make them "callable". You'll see many methods like: ``` @classmethod @internal_api_call def function_name(self, ...) return _function_name(...) ``` The goal is to move back the implementation of `_function_name` into `function_name`. > Also, what should I do if _fun() is called more than once like ./airflow/dag_processing/processor.py: _execute_task_callbacks? (it's my first time in the API area) In this case, (please @potiuk correct me if I am wrong) I dont think we should re-join this method. `_execute_task_callbacks` is called in different methods and the separation makes sense. > What should we do about these functions in rcp_api.py? Rename them to their original? Are you talking about `airflow/api_internal/endpoints/rpc_api_endpoint.py`? This file has been removed recently, so no need to take care of it > It's my first time in the API area, so I'll try not to break too many things :) No worries! Thanks for doing it! We all have to start someday right? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove `internal_api_call` decorator [airflow]
shahar1 commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508235669 > As part of this task, #44436 mentions as well "re-join back methods that were separated out from the main code - when methods start with _". Are you planning to do it in this PR? I'd advice to either do it as part of this PR or do the re-join before removing the decorator. Once the decorator is removed, it will be harder to look for methods that were used to be used for AIP-44 and needs to be re-joined Yes, that's why I just drafted it :) I'll take care of it, but just to make sure that I understand "re-join" correctly - everywhere where there's both `def fun()` and `def _fun()`, simply restore the logic from `_fun()` to `fun()`, or is it deeper than that? (it's my first time in the API area) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Remove `internal_api_call` decorator [airflow]
vincbeck commented on PR #44486: URL: https://github.com/apache/airflow/pull/44486#issuecomment-2508213697 As part of this task, #44436 mentions as well "re-join back methods that were separated out from the main code - when methods start with _". Are you planning to do it in this PR? I'd advice to either do it as part of this PR or do the re-join before removing the decorator. Once the decorator is removed, it will be harder to look for methods that were used to be used for AIP-44 and needs to be re-joined -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
