You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "eumiro (via GitHub)" <gi...@apache.org> on 2023/09/03 20:12:53 UTC
[GitHub] [airflow] eumiro opened a new pull request, #34060: Consolidate importing of os.path.*
eumiro opened a new pull request, #34060:
URL: https://github.com/apache/airflow/pull/34060
(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
[GitHub] [airflow] potiuk commented on pull request #34060: Consolidate importing of os.path.*
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34060:
URL: https://github.com/apache/airflow/pull/34060#issuecomment-1705650611
pending @uranusjr chaanges.
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314517696
##########
airflow/www/extensions/init_views.py:
##########
@@ -253,15 +254,15 @@ def _handle_method_not_allowed(ex):
else:
return views.method_not_allowed(ex)
- with open(path.join(ROOT_APP_DIR, "api_connexion", "openapi", "v1.yaml")) as f:
+ with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
specification = safe_load(f)
api_bp = FlaskApi(
specification=specification,
resolver=_LazyResolver(),
base_path=base_path,
options={
"swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- "swagger_path": path.join(ROOT_APP_DIR, "www", "static", "dist", "swagger-ui"),
+ "swagger_path": os.fspath(ROOT_APP_DIR / "www" / "static" / "dist" / "swagger-ui"),
Review Comment:
```suggestion
"swagger_path": os.fspath(ROOT_APP_DIR / "www/static/dist/swagger-ui"),
```
The performance difference is not insignificant on its own due to the fact that every `/` needs to go through a magic method (it’s not much in the grand scheme of things of course).
```python
py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www" / "static" / "dist" / "swagger-ui"'
50000 loops, best of 5: 5.65 usec per loop
$ py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www/static/dist/swagger-ui"'
200000 loops, best of 5: 1.37 usec per loop
```
Alternatively `joinpath` is also much better for this.
##########
airflow/www/extensions/init_views.py:
##########
@@ -253,15 +254,15 @@ def _handle_method_not_allowed(ex):
else:
return views.method_not_allowed(ex)
- with open(path.join(ROOT_APP_DIR, "api_connexion", "openapi", "v1.yaml")) as f:
+ with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
specification = safe_load(f)
api_bp = FlaskApi(
specification=specification,
resolver=_LazyResolver(),
base_path=base_path,
options={
"swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- "swagger_path": path.join(ROOT_APP_DIR, "www", "static", "dist", "swagger-ui"),
+ "swagger_path": os.fspath(ROOT_APP_DIR / "www" / "static" / "dist" / "swagger-ui"),
Review Comment:
```suggestion
"swagger_path": os.fspath(ROOT_APP_DIR / "www/static/dist/swagger-ui"),
```
The performance difference is not insignificant on its own due to the fact that every `/` needs to go through a magic method (it’s not much in the grand scheme of things of course).
```python
$ py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www" / "static" / "dist" / "swagger-ui"'
50000 loops, best of 5: 5.65 usec per loop
$ py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www/static/dist/swagger-ui"'
200000 loops, best of 5: 1.37 usec per loop
```
Alternatively `joinpath` is also much better for 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
[GitHub] [airflow] uranusjr commented on a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314647254
##########
scripts/ci/pre_commit/pre_commit_check_setup_extra_packages_ref.py:
##########
@@ -24,18 +24,18 @@
import os
import re
import sys
-from os.path import dirname
+from pathlib import Path
from rich import print
from rich.console import Console
from rich.table import Table
-AIRFLOW_SOURCES_DIR = os.path.join(dirname(__file__), os.pardir, os.pardir, os.pardir)
+AIRFLOW_SOURCES_DIR = Path(__file__).resolve().parents[3]
Review Comment:
That’d be awesome, although we can focus on the lines changed in this PR first.
##########
scripts/ci/pre_commit/pre_commit_check_setup_extra_packages_ref.py:
##########
@@ -24,18 +24,18 @@
import os
import re
import sys
-from os.path import dirname
+from pathlib import Path
from rich import print
from rich.console import Console
from rich.table import Table
-AIRFLOW_SOURCES_DIR = os.path.join(dirname(__file__), os.pardir, os.pardir, os.pardir)
+AIRFLOW_SOURCES_DIR = Path(__file__).resolve().parents[3]
Review Comment:
That’d be awesome, although we can focus on the lines changed in this PR first.
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314514610
##########
airflow/www/extensions/init_views.py:
##########
@@ -39,7 +40,7 @@
log = logging.getLogger(__name__)
# airflow/www/extensions/init_views.py => airflow/
-ROOT_APP_DIR = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
+ROOT_APP_DIR = Path(__file__).resolve().parents[2]
Review Comment:
```suggestion
ROOT_APP_DIR = Path(__file__).parents[2].resolve()
```
There’s a difference if any parent is a symlink. This better matches the original semantic.
##########
airflow/www/extensions/init_views.py:
##########
@@ -39,7 +40,7 @@
log = logging.getLogger(__name__)
# airflow/www/extensions/init_views.py => airflow/
-ROOT_APP_DIR = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
+ROOT_APP_DIR = Path(__file__).resolve().parents[2]
Review Comment:
```suggestion
ROOT_APP_DIR = Path(__file__).parents[2].resolve()
```
There’s a difference if any parent is a symlink. This better matches the original semantic.
##########
airflow/www/extensions/init_views.py:
##########
@@ -39,7 +40,7 @@
log = logging.getLogger(__name__)
# airflow/www/extensions/init_views.py => airflow/
-ROOT_APP_DIR = path.abspath(path.join(path.dirname(__file__), path.pardir, path.pardir))
+ROOT_APP_DIR = Path(__file__).resolve().parents[2]
Review Comment:
```suggestion
ROOT_APP_DIR = Path(__file__).parents[2].resolve()
```
There’s a difference if any parent is a symlink. This better matches the original (and intended) semantic.
--
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] eumiro commented on a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314631178
##########
scripts/ci/pre_commit/pre_commit_inline_scripts_in_docker.py:
##########
@@ -45,16 +44,16 @@ def insert_content(file_path: Path, content: list[str], header: str, footer: str
SCRIPTS_DOCKER_DIR = AIRFLOW_SOURCES_DIR / "scripts" / "docker"
for file in [DOCKERFILE_FILE, DOCKERFILE_CI_FILE]:
- for script in listdir(SCRIPTS_DOCKER_DIR):
- script_content = (SCRIPTS_DOCKER_DIR / script).read_text().splitlines(keepends=True)
+ for script in SCRIPTS_DOCKER_DIR.listdir():
Review Comment:
Should by `iterdir()` actually.
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314532886
##########
scripts/ci/pre_commit/pre_commit_check_order_setup.py:
##########
@@ -26,15 +26,14 @@
import re
import sys
import textwrap
-from os.path import abspath, dirname
+from pathlib import Path
from rich import print
errors: list[str] = []
-MY_DIR_PATH = os.path.dirname(__file__)
-SOURCE_DIR_PATH = os.path.abspath(os.path.join(MY_DIR_PATH, os.pardir, os.pardir, os.pardir))
-sys.path.insert(0, SOURCE_DIR_PATH)
+SOURCE_DIR_PATH = Path(__file__).resolve().parents[3]
Review Comment:
```suggestion
SOURCE_DIR_PATH = Path(__file__).parents[3].resolve()
```
Since `resolve` needs to be called anyway this can actually be
```suggestion
SOURCE_DIR_PATH = Path(__file__, "..", "..", "..").resolve()
```
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314517696
##########
airflow/www/extensions/init_views.py:
##########
@@ -253,15 +254,15 @@ def _handle_method_not_allowed(ex):
else:
return views.method_not_allowed(ex)
- with open(path.join(ROOT_APP_DIR, "api_connexion", "openapi", "v1.yaml")) as f:
+ with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
specification = safe_load(f)
api_bp = FlaskApi(
specification=specification,
resolver=_LazyResolver(),
base_path=base_path,
options={
"swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- "swagger_path": path.join(ROOT_APP_DIR, "www", "static", "dist", "swagger-ui"),
+ "swagger_path": os.fspath(ROOT_APP_DIR / "www" / "static" / "dist" / "swagger-ui"),
Review Comment:
```suggestion
"swagger_path": os.fspath(ROOT_APP_DIR / "www/static/dist/swagger-ui"),
```
The performance difference is not insignificant on its own due to the fact that every `/` needs to go through a magic method (it’s not much in the grand scheme of things of course).
```python
py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www" / "static" / "dist" / "swagger-ui"'
50000 loops, best of 5: 5.65 usec per loop
uranusjr@Stuuuu airflow $ py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www/static/dist/swagger-ui"'
200000 loops, best of 5: 1.37 usec per loop
```
Alternatively `joinpath` is also much better for this.
##########
airflow/www/extensions/init_views.py:
##########
@@ -253,15 +254,15 @@ def _handle_method_not_allowed(ex):
else:
return views.method_not_allowed(ex)
- with open(path.join(ROOT_APP_DIR, "api_connexion", "openapi", "v1.yaml")) as f:
+ with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
specification = safe_load(f)
api_bp = FlaskApi(
specification=specification,
resolver=_LazyResolver(),
base_path=base_path,
options={
"swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- "swagger_path": path.join(ROOT_APP_DIR, "www", "static", "dist", "swagger-ui"),
+ "swagger_path": os.fspath(ROOT_APP_DIR / "www" / "static" / "dist" / "swagger-ui"),
Review Comment:
```suggestion
"swagger_path": os.fspath(ROOT_APP_DIR / "www/static/dist/swagger-ui"),
```
The performance difference is not insignificant on its own due to the fact that every `/` needs to go through a magic method (it’s not much in the grand scheme of things of course).
```python
py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www" / "static" / "dist" / "swagger-ui"'
50000 loops, best of 5: 5.65 usec per loop
uranusjr@Stuuuu airflow $ py -m timeit -s 'from pathlib import Path; p = Path.cwd()' 'p / "www/static/dist/swagger-ui"'
200000 loops, best of 5: 1.37 usec per loop
```
Alternatively `joinpath` is also much better for 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
[GitHub] [airflow] uranusjr commented on a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314532886
##########
scripts/ci/pre_commit/pre_commit_check_order_setup.py:
##########
@@ -26,15 +26,14 @@
import re
import sys
import textwrap
-from os.path import abspath, dirname
+from pathlib import Path
from rich import print
errors: list[str] = []
-MY_DIR_PATH = os.path.dirname(__file__)
-SOURCE_DIR_PATH = os.path.abspath(os.path.join(MY_DIR_PATH, os.pardir, os.pardir, os.pardir))
-sys.path.insert(0, SOURCE_DIR_PATH)
+SOURCE_DIR_PATH = Path(__file__).resolve().parents[3]
Review Comment:
```suggestion
SOURCE_DIR_PATH = Path(__file__).parents[3].resolve()
```
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314636857
##########
scripts/ci/pre_commit/pre_commit_check_setup_extra_packages_ref.py:
##########
@@ -24,18 +24,18 @@
import os
import re
import sys
-from os.path import dirname
+from pathlib import Path
from rich import print
from rich.console import Console
from rich.table import Table
-AIRFLOW_SOURCES_DIR = os.path.join(dirname(__file__), os.pardir, os.pardir, os.pardir)
+AIRFLOW_SOURCES_DIR = Path(__file__).resolve().parents[3]
Review Comment:
```suggestion
AIRFLOW_SOURCES_DIR = Path(__file__).parents[3].resolve()
```
or
```suggestion
AIRFLOW_SOURCES_DIR = Path(__file__).parents[3]
```
since the original code seems to work without resolving 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] potiuk merged pull request #34060: Consolidate importing of os.path.*
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #34060:
URL: https://github.com/apache/airflow/pull/34060
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314635437
##########
scripts/ci/pre_commit/pre_commit_inline_scripts_in_docker.py:
##########
@@ -45,16 +44,16 @@ def insert_content(file_path: Path, content: list[str], header: str, footer: str
SCRIPTS_DOCKER_DIR = AIRFLOW_SOURCES_DIR / "scripts" / "docker"
for file in [DOCKERFILE_FILE, DOCKERFILE_CI_FILE]:
- for script in listdir(SCRIPTS_DOCKER_DIR):
- script_content = (SCRIPTS_DOCKER_DIR / script).read_text().splitlines(keepends=True)
+ for script in SCRIPTS_DOCKER_DIR.listdir():
Review Comment:
Oh right, I was confusing this with `os.scandir`.
--
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] eumiro commented on a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314640263
##########
scripts/ci/pre_commit/pre_commit_check_setup_extra_packages_ref.py:
##########
@@ -24,18 +24,18 @@
import os
import re
import sys
-from os.path import dirname
+from pathlib import Path
from rich import print
from rich.console import Console
from rich.table import Table
-AIRFLOW_SOURCES_DIR = os.path.join(dirname(__file__), os.pardir, os.pardir, os.pardir)
+AIRFLOW_SOURCES_DIR = Path(__file__).resolve().parents[3]
Review Comment:
There are 13 occurrences of `resolve().parents` in the repo. Should we fix them all?
--
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 a diff in pull request #34060: Consolidate importing of os.path.*
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34060:
URL: https://github.com/apache/airflow/pull/34060#discussion_r1314535313
##########
scripts/ci/pre_commit/pre_commit_inline_scripts_in_docker.py:
##########
@@ -45,16 +44,16 @@ def insert_content(file_path: Path, content: list[str], header: str, footer: str
SCRIPTS_DOCKER_DIR = AIRFLOW_SOURCES_DIR / "scripts" / "docker"
for file in [DOCKERFILE_FILE, DOCKERFILE_CI_FILE]:
- for script in listdir(SCRIPTS_DOCKER_DIR):
- script_content = (SCRIPTS_DOCKER_DIR / script).read_text().splitlines(keepends=True)
+ for script in SCRIPTS_DOCKER_DIR.listdir():
Review Comment:
Use `scandir` instead
--
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