You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/03/17 16:04:00 UTC

[GitHub] [airflow] norm opened a new pull request #22341: List mapped tis

norm opened a new pull request #22341:
URL: https://github.com/apache/airflow/pull/22341


   <!--
   Thank you for contributing! Please make sure that your code changes
   are covered with tests. And in case of new features or big changes
   remember to adjust the documentation.
   
   Feel free to ping committers for the review!
   
   In case of existing issue, reference it using one of the following:
   
   closes: #ISSUE
   related: #ISSUE
   
   How to write a good git commit message:
   http://chris.beams.io/posts/git-commit/
   -->
   
   ---
   **^ Add meaningful description above**
   
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
   


-- 
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

[GitHub] [airflow] bbovenzi edited a comment on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1076994331


   I've been testing locally. Pagination and ordering seems to be working well! Let's just get checks passing and this is good for me.


-- 
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

[GitHub] [airflow] bbovenzi commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1074204677


   Could you add a screenshot of the OpenAPI rendered yaml file to the PR description? It would help to quickly see if the API has everything needed for people to use it.


-- 
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

[GitHub] [airflow] norm commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1076388493


   @uranusjr I think if it was never mapped (as opposed to 0 mapped tasks being created), it should return a 404. Not sure how to detect the difference, however.


-- 
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

[GitHub] [airflow] norm commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1075333022


   > Could you add a screenshot of the OpenAPI rendered yaml file to the PR description? It would help to quickly see if the API has everything needed for people to use it.
   
   Added, and I'll hopefully remember to do that in future without prompting :)


-- 
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

[GitHub] [airflow] uranusjr commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1076514591


   A task never mapped (either due to it not being a mapped task, or has not been run yet) would have one single task instance of `map_index=-1`. If the task is mapped, there will never be an instance with the -1 index. If the mapped length is zero, there will only be one task instance with index 0 and marked with state `SKIPPED`.


-- 
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

[GitHub] [airflow] norm commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1077285031


   @uranusjr From my reading of the code, when the mapped length is 0, the state is set to skipped but the map_index is not altered to 0 before the return (also what my tests show when I debug them). Is this a bug in `mappedoperator.py` I should fix?


-- 
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

[GitHub] [airflow] ashb merged pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
ashb merged pull request #22341:
URL: https://github.com/apache/airflow/pull/22341


   


-- 
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

[GitHub] [airflow] ashb commented on a change in pull request #22341: List mapped tis

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#discussion_r829861390



##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1109,6 +1109,39 @@ paths:
         '404':
           $ref: '#/components/responses/NotFound'
 
+  /dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/list:

Review comment:
       ```suggestion
     /dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/listMapped:
   ```




-- 
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

[GitHub] [airflow] ashb commented on a change in pull request #22341: List mapped tis

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#discussion_r829861390



##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1109,6 +1109,39 @@ paths:
         '404':
           $ref: '#/components/responses/NotFound'
 
+  /dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/list:

Review comment:
       ```suggestion
     /dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/listMapped:
   ```




-- 
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

[GitHub] [airflow] github-actions[bot] commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1078572197


   The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.


-- 
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

[GitHub] [airflow] uranusjr commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1077480944


   Hmm, I’m not sure. I remember we discussed marking the task instance as skipped, but not sure if we mentioned if that skipped task instance needs to be set to index 0. And this does not really matter anywhere anyway, so I think we can choose whatever makes things easier here.


-- 
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

[GitHub] [airflow] norm commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1077507588


   Since there would still be ambiguity in trying to figure it out from just the TI when both leaving it -1 and changing it to 0, checking `is_mapped` in the DAG seems the best way of resolving things. I'll try 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

[GitHub] [airflow] ashb commented on a change in pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#discussion_r836302259



##########
File path: tests/api_connexion/endpoints/test_task_instance_endpoint.py
##########
@@ -376,6 +376,16 @@ def test_should_return_404_for_mapped_endpoint(self, session):
             )
             assert response.status_code == 404
 
+    def test_should_return_404_for_list_mapped_endpoint(self, session):
+        self.create_task_instances(session)
+        for index in ['0', '1', '2']:
+            response = self.client.get(
+                "/api/v1/dags/example_python_operator/dagRuns/TEST_DAG_RUN_ID/"
+                "taskInstances/print_the_context/listMapped",
+                environ_overrides={"REMOTE_USER": "test"},
+            )
+            assert response.status_code == 404

Review comment:
       I don't think this for loop is needed/does anything now
   ```suggestion
           response = self.client.get(
               "/api/v1/dags/example_python_operator/dagRuns/TEST_DAG_RUN_ID/"
               "taskInstances/print_the_context/listMapped",
               environ_overrides={"REMOTE_USER": "test"},
           )
           assert response.status_code == 404
   ```




-- 
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

[GitHub] [airflow] norm commented on a change in pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
norm commented on a change in pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#discussion_r837073475



##########
File path: tests/api_connexion/endpoints/test_task_instance_endpoint.py
##########
@@ -376,6 +376,16 @@ def test_should_return_404_for_mapped_endpoint(self, session):
             )
             assert response.status_code == 404
 
+    def test_should_return_404_for_list_mapped_endpoint(self, session):
+        self.create_task_instances(session)
+        for index in ['0', '1', '2']:
+            response = self.client.get(
+                "/api/v1/dags/example_python_operator/dagRuns/TEST_DAG_RUN_ID/"
+                "taskInstances/print_the_context/listMapped",
+                environ_overrides={"REMOTE_USER": "test"},
+            )
+            assert response.status_code == 404

Review comment:
       Yep, that was thoughtless copy+paste on my part.




-- 
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

[GitHub] [airflow] uranusjr commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1076369356


   I wonder if it makes more sense to return an empty list (current behaviour) or 404 Not Found if this is called on a non-mapped task.


-- 
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

[GitHub] [airflow] ashb commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1077490856


   I think that if we find no mapped TIs we should load the dag and check, because there it might be worth being able to differentiate between:
   
   - mapped item was zero length, whole thing was skipped
   - mapped item returned one item, and that one task execution chose to skip.
   
   We could either load the DAG and check if `task.is_mapped`? Or just load all TIs (no map_index SQL filtering) and check things further only if we find an index of -1 (so that the "default" case of called on an expanded mapped index can run without extra queries, as loading the DAG is not free.)


-- 
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

[GitHub] [airflow] norm commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1072636051


   Needs to accept order and search params.


-- 
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

[GitHub] [airflow] bbovenzi commented on pull request #22341: Add API endpoint to list mapped task instances

Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #22341:
URL: https://github.com/apache/airflow/pull/22341#issuecomment-1076994331


   I've been testing locally. Pagination and ordering seems to be working well!


-- 
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