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/12/07 05:02:33 UTC

[GitHub] [airflow] aleksandr-shel opened a new pull request, #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

aleksandr-shel opened a new pull request, #28183:
URL: https://github.com/apache/airflow/pull/28183

   • this commit allows users to write dag_id and task_id in their national characters, which are usually non-ASCIIs as the display names of their dags. 
   • dag_id and task_id will be modified to ASCII version with the usage of [slugify](https://github.com/mozilla/unicode-slugify)  and stored in dag_id or task_id fields of dag and task
   • unmodified version (possibly non-ASCII) of dag_id and task_id will be stored in display_name fields of dag and task
   • related: #22073 
   


-- 
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] closed pull request #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] closed pull request #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task
URL: https://github.com/apache/airflow/pull/28183


-- 
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] potiuk commented on pull request #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

Posted by GitBox <gi...@apache.org>.

potiuk commented on PR #28183:
URL: https://github.com/apache/airflow/pull/28183#issuecomment-1367920739

I like how the implementation is started (and I think this one is needed). Particularly, I like it's going to be 100% backwards compatible and will only impact new DAGs with new DAG_IDs.

I know however that there are people who have rather strong opinion on the subject so I will leave it for others to comment @ashb @uranusjr @dstandish @eladkal - I think it would be good to get your comments here.

And I would like to get more comments, before we decide to progress with this idea because there are a LOT more to be done in this PR if it is going to be approved.

But if we do, the more changes needed are I believe:

* track down and update all the places where dag_id is displayed in the UI. I have a feeling that the current proposal is a very small subset of those and we cannot affort inconsistently displaying slugified/non-slugified names. And we should make a decision whether we display only the display id or maybe both of them.

* IMHO we need to make sure that you can use either slugified or non-slugified dag_id (i.e. display name) everywhere where there is a user interaction involved: CLI, API, possibly also in the places where "public interface" is used - for example when there is an XCom retrieval). Otherwise we risk a lot of confusion from the user's point of view. It's extremely bad idea to expect the users to have to manually run slugify (and know how to do it) when they want to query or interface with dag that they only know from their display name. There might be more cases where this aspect should be considered (and if we decide to everywhere display both - display name and id in case they differ, this one might be not needed.

* We likely need to handle and detect the case where two different non-slugified dag_ids produce the same slugified dag_ids. This will become much more difficult to track if we do slugification. Likely DAGFileProcessor should detect such case (by comparing display_name and dag_id) and if there is an existing dag_id with different display name, detected a new kind of error should be displayed for the users (similar to but different from ImportError)

All those should have comprehensive unit tests covering all the edge cases there before we approve it anyway.

--
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 #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #28183:
URL: https://github.com/apache/airflow/pull/28183#issuecomment-1445512593

   This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.


-- 
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] xgao1023 commented on pull request #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

Posted by "xgao1023 (via GitHub)" <gi...@apache.org>.

xgao1023 commented on PR #28183:
URL: https://github.com/apache/airflow/pull/28183#issuecomment-1630536728

   Is slugifying the IDs the only concern?
   
   If so I will be happy to remove that part - task/dag ID should just work as before - either explicitly provided or from the function name of task flow.
   
   I will be happy to continue the work if it makes sense.


-- 
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] boring-cyborg[bot] commented on pull request #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on PR #28183:
URL: https://github.com/apache/airflow/pull/28183#issuecomment-1340376814

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

--
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 #28183: allows users to write dag_id and task_id in their national characters, added display name for dag / task

Posted by GitBox <gi...@apache.org>.

uranusjr commented on PR #28183:
URL: https://github.com/apache/airflow/pull/28183#issuecomment-1369406739

   I like the general direction this takes. The only part I _don’t_ like is it silently slugifies the IDs. This does create some backward incompatibility issues (can be eliminated if we only slugify if the ID is not valid as-is), but more importantly, can result in som weird usability issues, such as ID conflicts when there are no actually conflicting IDs (because the slugified IDs conflict). This could be improved by improving error messages to report the original (user-supplied) value instead of the slugified one. Another further issue with this is it won’t be possible for the user to work around this slugified ID conflict. One possible solution would be to make the API more explicit, such as:
   
   ```python
   # This would generate an auto slugified ID.
   DAG(dag_name="いろはにほへと")
   
   # But I can supply my own ID.
   DAG(dag_name="いろはにほへと", dag_id="my_dag")
   ```


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