Re: [PR] Use relative path in ParseImportError.filename [airflow]
ephraimbuddy merged PR #51406: URL: https://github.com/apache/airflow/pull/51406 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Use relative path in ParseImportError.filename [airflow]
uranusjr commented on code in PR #51406:
URL: https://github.com/apache/airflow/pull/51406#discussion_r2141672632
##
airflow-core/src/airflow/dag_processing/manager.py:
##
@@ -1141,11 +1141,16 @@ def process_parse_results(
stat.import_errors = 1
else:
# record DAGs and import errors to database
+import_errors = {}
+if parsing_result.import_errors:
+import_errors = {
+(bundle_name, rel_path): error for rel_path, error in
parsing_result.import_errors.items()
+}
Review Comment:
```suggestion
import_errors = {
(bundle_name, rel_path): error for rel_path, error in
parsing_result.import_errors.items()
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Use relative path in ParseImportError.filename [airflow]
uranusjr commented on code in PR #51406: URL: https://github.com/apache/airflow/pull/51406#discussion_r2141941071 ## airflow-core/src/airflow/dag_processing/collection.py: ## @@ -254,51 +261,67 @@ def _update_import_errors( session.execute( delete(ParseImportError).where( -ParseImportError.filename.in_(list(files_parsed)), ParseImportError.bundle_name == bundle_name +tuple_(ParseImportError.bundle_name, ParseImportError.filename).in_(list(files_parsed)) Review Comment: ```suggestion tuple_(ParseImportError.bundle_name, ParseImportError.filename).in_(files_parsed) ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Use relative path in ParseImportError.filename [airflow]
uranusjr commented on code in PR #51406:
URL: https://github.com/apache/airflow/pull/51406#discussion_r2141672632
##
airflow-core/src/airflow/dag_processing/manager.py:
##
@@ -1141,11 +1141,16 @@ def process_parse_results(
stat.import_errors = 1
else:
# record DAGs and import errors to database
+import_errors = {}
+if parsing_result.import_errors:
+import_errors = {
+(bundle_name, rel_path): error for rel_path, error in
parsing_result.import_errors.items()
+}
Review Comment:
```suggestion
import_errors = {
(bundle_name, rel_path): error for rel_path, error in
parsing_result.import_errors.items()
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Use relative path in ParseImportError.filename [airflow]
uranusjr commented on code in PR #51406: URL: https://github.com/apache/airflow/pull/51406#discussion_r2141668796 ## airflow-core/src/airflow/api_fastapi/core_api/routes/public/import_error.py: ## @@ -152,14 +152,20 @@ def get_import_errors( readable_dag_ids = auth_manager.get_authorized_dag_ids(method="GET", user=user) # Build a cte that fetches dag_ids for each file location visiable_files_cte = ( -select(DagModel.fileloc, DagModel.dag_id).where(DagModel.dag_id.in_(readable_dag_ids)).cte() +select(DagModel.relative_fileloc, DagModel.dag_id, DagModel.bundle_name) +.where(DagModel.dag_id.in_(readable_dag_ids)) +.cte() ) # Prepare the import errors query by joining with the cte. # Each returned row will be a tuple: (ParseImportError, dag_id) import_errors_stmt = ( select(ParseImportError, visiable_files_cte.c.dag_id) -.join(visiable_files_cte, ParseImportError.filename == visiable_files_cte.c.fileloc) +.join( +visiable_files_cte, Review Comment: Most likely just a typo. Let’s fix 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Use relative path in ParseImportError.filename [airflow]
bugraoz93 commented on code in PR #51406: URL: https://github.com/apache/airflow/pull/51406#discussion_r2141107651 ## airflow-core/src/airflow/api_fastapi/core_api/routes/public/import_error.py: ## @@ -152,14 +152,20 @@ def get_import_errors( readable_dag_ids = auth_manager.get_authorized_dag_ids(method="GET", user=user) # Build a cte that fetches dag_ids for each file location visiable_files_cte = ( -select(DagModel.fileloc, DagModel.dag_id).where(DagModel.dag_id.in_(readable_dag_ids)).cte() +select(DagModel.relative_fileloc, DagModel.dag_id, DagModel.bundle_name) +.where(DagModel.dag_id.in_(readable_dag_ids)) +.cte() ) # Prepare the import errors query by joining with the cte. # Each returned row will be a tuple: (ParseImportError, dag_id) import_errors_stmt = ( select(ParseImportError, visiable_files_cte.c.dag_id) -.join(visiable_files_cte, ParseImportError.filename == visiable_files_cte.c.fileloc) +.join( +visiable_files_cte, Review Comment: I am not sure if it was intentionally named like this in the past. Maybe we can rename it ```suggestion visible_files_cte, ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Use relative path in ParseImportError.filename [airflow]
Copilot commented on code in PR #51406: URL: https://github.com/apache/airflow/pull/51406#discussion_r2131789311 ## airflow-core/src/airflow/dag_processing/collection.py: ## @@ -254,51 +261,65 @@ def _update_import_errors( session.execute( delete(ParseImportError).where( -ParseImportError.filename.in_(list(files_parsed)), ParseImportError.bundle_name == bundle_name +tuple_(ParseImportError.bundle_name, ParseImportError.filename).in_(list(files_parsed)) ) ) existing_import_error_files = set( -session.execute(select(ParseImportError.filename, ParseImportError.bundle_name)) +session.execute(select(ParseImportError.bundle_name, ParseImportError.filename)) Review Comment: [nitpick] Since the update now relies on matching tuple keys from the database with those in import_errors, consider adding an inline comment explaining the expected structure of the results for clarity and future maintainability. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
