Add `os.PathLike` when filepath as `str` is expected #5200

vahvero · 2023-05-04T17:11:08Z

Description

closes #5182 by adding os.PathLike to type unions and using os.fspath at some points.

Motivation and Context

Context

onnx file handling accepts only IO[bytes] | str. If passing Python pathlib.Path, static type checkers such as pyright and pylance emit errors which is incorrect.

import onnx
import pathlib

model = onnx.load(pathlib.Path("model.onnx"))

Causes PyRight error.

Motivation

Using pathlib to handle paths is considered best practice by many.
Would remove unnecessary object transformations to satisfy type checkers
Reduces # type: ignore usage in user code base.
These, in general, should benefit general code quality.

Initial issue discussion was with

justinchuby

This is my first time contributing to anything public. Changes are very minor and any feedback is welcome.

justinchuby

Thanks! Some unit tests will be helpful. You may consider adding them to https://github.com/onnx/onnx/blob/main/onnx/test/basic_test.py and https://github.com/onnx/onnx/blob/main/onnx/test/test_external_data.py

onnx/__init__.py

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero · 2023-05-06T12:09:08Z

I have some issues trying to build ONNX from source to run local tests.

Obtaining file:///home/vahvero/Documents/onnx
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Checking if build backend supports build_editable: started
  Checking if build backend supports build_editable: finished with status 'done'
  Getting requirements to build editable: started
  Getting requirements to build editable: finished with status 'done'
  Installing backend dependencies: started
  Installing backend dependencies: finished with status 'done'
  Preparing editable metadata (pyproject.toml): started
  Preparing editable metadata (pyproject.toml): finished with status 'done'
Collecting numpy (from onnx==1.15.0)
  Using cached numpy-1.24.3-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (17.3 MB)
Collecting protobuf>=3.20.2 (from onnx==1.15.0)
  Using cached protobuf-4.22.4-cp37-abi3-manylinux2014_x86_64.whl (302 kB)
Collecting typing-extensions>=3.6.2.1 (from onnx==1.15.0)
  Using cached typing_extensions-4.5.0-py3-none-any.whl (27 kB)
Building wheels for collected packages: onnx
  Building editable for onnx (pyproject.toml): started
  Building editable for onnx (pyproject.toml): finished with status 'error'
  error: subprocess-exited-with-error
  
  × Building editable for onnx (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [2824 lines of output]
      running editable_wheel
      creating /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info
      writing /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/PKG-INFO
      writing dependency_links to /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/dependency_links.txt
      writing entry points to /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/entry_points.txt
      writing requirements to /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/requires.txt
      writing top-level names to /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/top_level.txt
      writing manifest file '/tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/SOURCES.txt'
      reading manifest file '/tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/SOURCES.txt'
      reading manifest template 'MANIFEST.in'
      warning: no files found matching '*.c' under directory 'onnx'
      adding license file 'LICENSE'
      writing manifest file '/tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx.egg-info/SOURCES.txt'
      creating '/tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx-1.15.0.dist-info'
      creating /tmp/pip-wheel-zosivw_u/.tmp-s7s3loqj/onnx-1.15.0.dist-info/WHEEL
      running build_py
      running create_version
      running cmake_build
      Extra cmake args: ['-DONNX_USE_LITE_PROTO=ON']
      Using cmake args: ['/usr/bin/cmake', '-DPYTHON_INCLUDE_DIR=/home/vahvero/.pyenv/versions/3.9.16/include/python3.9', '-DPYTHON_EXECUTABLE=/home/vahvero/Documents/onnx/venv/bin/python3.9', '-DBUILD_ONNX_PYTHON=ON', '-DCMAKE_EXPORT_COMPILE_COMMANDS=ON', '-DONNX_NAMESPACE=onnx', '-DPY_EXT_SUFFIX=.cpython-39-x86_64-linux-gnu.so', '-DCMAKE_BUILD_TYPE=Release', '-DONNX_ML=1', '-DONNX_USE_LITE_PROTO=ON', '/home/vahvero/Documents/onnx']
      CMake Warning at /usr/share/cmake-3.25/Modules/FindProtobuf.cmake:524 (message):
        Protobuf compiler version 22.3 doesn't match library version 3.20.2
      Call Stack (most recent call first):
        CMakeLists.txt:179 (find_package)
      
      
      Generated: /home/vahvero/Documents/onnx/.setuptools-cmake-build/onnx/onnx-ml.proto
      Generated: /home/vahvero/Documents/onnx/.setuptools-cmake-build/onnx/onnx-operators-ml.proto
      Generated: /home/vahvero/Documents/onnx/.setuptools-cmake-build/onnx/onnx-data.proto
      -- Could NOT find pybind11 (missing: pybind11_DIR)
      -- pybind11 v2.10.3
      --
      -- ******** Summary ********
      --   CMake version             : 3.25.1
      --   CMake command             : /usr/bin/cmake
      --   System                    : Linux
      --   C++ compiler              : /usr/bin/c++
      --   C++ compiler version      : 12.2.0
      --   CXX flags                 :  -Wnon-virtual-dtor
      --   Build type                : Release
      --   Compile definitions       : __STDC_FORMAT_MACROS
      --   CMAKE_PREFIX_PATH         :
      --   CMAKE_INSTALL_PREFIX      : /usr/local
      --   CMAKE_MODULE_PATH         :
      --
      --   ONNX version              : 1.15.0
      --   ONNX NAMESPACE            : onnx
      --   ONNX_USE_LITE_PROTO       : ON
<continues for a while>

                File "/home/vahvero/.pyenv/versions/3.9.16/lib/python3.9/subprocess.py", line 373, in check_call
                  raise CalledProcessError(retcode, cmd)
              subprocess.CalledProcessError: Command '['/usr/bin/cmake', '--build', '.', '--', '-j', '12']' returned non-zero exit status 2.
      
      
                              If you are seeing this warning it is very likely that a setuptools
                              plugin or customization overrides the `build_py` command, without
                              taking into consideration how editable installs run build steps
                              starting from setuptools v64.0.0.
      
                              Plugin authors and developers relying on custom build steps are
                              encouraged to update their `build_py` implementation considering the
                              information about editable installs in
                              https://setuptools.pypa.io/en/latest/userguide/extension.html.
      
                              For the time being `setuptools` will silence this error and ignore
                              the faulty command, but this behaviour will change in future versions.
      
              ********************************************************************************
      
      !!
        self._safely_run(name)
      running build_ext
      running cmake_build
      Traceback (most recent call last):
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/command/editable_wheel.py", line 155, in run
          self._create_wheel_file(bdist_wheel)
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/command/editable_wheel.py", line 344, in _create_wheel_file
          files, mapping = self._run_build_commands(dist_name, unpacked, lib, tmp)
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/command/editable_wheel.py", line 267, in _run_build_commands
          self._run_build_subcommands()
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/command/editable_wheel.py", line 294, in _run_build_subcommands
          self.run_command(name)
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/_distutils/cmd.py", line 318, in run_command
          self.distribution.run_command(command)
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/dist.py", line 1244, in run_command
          super().run_command(command)
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
          cmd_obj.run()
        File "<string>", line 260, in run
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/command/build_ext.py", line 84, in run
          _build_ext.run(self)
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/_distutils/command/build_ext.py", line 345, in run
          self.build_extensions()
        File "<string>", line 277, in build_extensions
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/_distutils/cmd.py", line 350, in copy_file
          return file_util.copy_file(
        File "/tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/_distutils/file_util.py", line 115, in copy_file
          raise DistutilsFileError(
      distutils.errors.DistutilsFileError: can't copy '/home/vahvero/Documents/onnx/.setuptools-cmake-build/onnx_cpp2py_export.cpython-39-x86_64-linux-gnu.so': doesn't exist or not a regular file
      /tmp/pip-build-env-hnr9o_ab/overlay/lib/python3.9/site-packages/setuptools/_distutils/dist.py:988: _DebuggingTips: Problem in editable installation.
      !!
      
              ********************************************************************************
              An error happened while installing `onnx` in editable mode.
      
              The following steps are recommended to help debug this problem:
      
              - Try to install the project normally, without using the editable mode.
                Does the error still persist?
                (If it does, try fixing the problem before attempting the editable mode).
              - If you are using binary extensions, make sure you have all OS-level
                dependencies installed (e.g. compilers, toolchains, binary libraries, ...).
              - Try the latest version of setuptools (maybe the error was already fixed).
              - If you (or your project dependencies) are using any setuptools extension
                or customization, make sure they support the editable mode.
      
              After following the steps above, if the problem still persists and
              you think this is related to how setuptools handles editable installations,
              please submit a reproducible example
              (see https://stackoverflow.com/help/minimal-reproducible-example) to:
      
                  https://github.com/pypa/setuptools/issues
      
              See https://setuptools.pypa.io/en/latest/userguide/development_mode.html for details.
              ********************************************************************************
      
      !!
        cmd_obj.run()
      error: can't copy '/home/vahvero/Documents/onnx/.setuptools-cmake-build/onnx_cpp2py_export.cpython-39-x86_64-linux-gnu.so': doesn't exist or not a regular file
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building editable for onnx
Failed to build onnx
ERROR: Could not build wheels for onnx, which is required to install pyproject.toml-based projects

Whole output

I am not experienced with large scale C++. I will try later again, installing pybind11 did not fix error.

>>> pip show pybind11
Name: pybind11
Version: 2.10.3
Summary: Seamless operability between C++11 and Python
Home-page: https://github.com/pybind/pybind11
Author: Wenzel Jakob
Author-email: wenzel.jakob@epfl.ch
License: BSD
Location: /home/vahvero/Documents/onnx/venv/lib/python3.9/site-packages
Requires: 
Required-by:

jcwchen

I have some issues trying to build ONNX from source to run local tests.

According to the error message, it seems that you have multiple Protobuf compiler. Please git clean -xdf to clean all build files, pip install --upgrade pip setuptools to get the latest setuptools, and then try pip install -e . again.

vahvero · 2023-05-13T11:47:38Z

I have some issues trying to build ONNX from source to run local tests.

According to the error message, it seems that you have multiple Protobuf compiler. Please git clean -xdf to clean all build files, pip install --upgrade pip setuptools to get the latest setuptools, and then try pip install -e . again.

I had installed protobuf from source, and copied it to /usr/local/bin/protoc. Removing this file removed the errors. Thank you.

onnx/test/basic_test.py

onnx/test/test_external_data.py

justinchuby · 2023-05-13T23:02:14Z

Please also make sure the change is free of lint errors: https://github.com/onnx/onnx/blob/2e9a6757ad33ef4bc0c4a4ecc61837f79885a82a/docs/CONTRIBUTING.md#code-style thanks a lot!

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero · 2023-05-14T17:04:44Z

Please also make sure the change is free of lint errors: https://github.com/onnx/onnx/blob/2e9a6757ad33ef4bc0c4a4ecc61837f79885a82a/docs/CONTRIBUTING.md#code-style thanks a lot!

I did not realize all pushes automatically request review. Should now pass all linters and tests, at least did in local.

Signed-off-by: vahvero <otso.brummer@gmail.com>

onnx/__init__.py

onnx/checker.py

onnx/test/test_external_data.py

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

onnx/__init__.py

onnx/utils.py

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

jcwchen

Thank you for extending! Could you please also update this function:

onnx/onnx/shape_inference.py

Lines 58 to 60 in c4a4327

    
           def infer_shapes_path( 
        
               model_path: str, 
        
               output_path: str = "",

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero · 2023-05-28T13:25:35Z

Thank you for extending! Could you please also update this function:

onnx/onnx/shape_inference.py

Lines 58 to 60 in c4a4327

def infer_shapes_path(

model_path: str,

output_path: str = "",

Included to 495be78

Signed-off-by: vahvero <otso.brummer@gmail.com>

justinchuby · 2023-05-30T01:03:23Z

You may want to run lintrunner -a to fix the rest of the lint errors

vahvero · 2023-05-30T06:33:06Z

You may want to run lintrunner -a to fix the rest of the lint errors

I receive no errors when running lintrunner. Are there any common configuration mistakes with it?

justinchuby · 2023-05-30T14:04:14Z

Maybe try lintrunner init to make sure all linters are up to date then run lintrunner --all -a to run it on all files?

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero · 2023-05-30T18:44:54Z

Maybe try lintrunner init to make sure all linters are up to date then run lintrunner --all -a to run it on all files?

Fixed in 14b4b7c

Signed-off-by: vahvero <otso.brummer@gmail.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com> Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Co-authored-by: Chun-Wei Chen <jacky82226@gmail.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>

vahvero requested a review from a team as a code owner May 4, 2023 17:11

vahvero force-pushed the main branch from d5e4464 to 3647998 Compare May 4, 2023 17:13

justinchuby reviewed May 4, 2023

View reviewed changes

onnx/__init__.py Outdated Show resolved Hide resolved

vahvero added 3 commits May 6, 2023 13:25

Added pathlike

439b37c

Signed-off-by: vahvero <otso.brummer@gmail.com>

Removed unnecessary os.fspath calls

b84d9dc

Signed-off-by: vahvero <otso.brummer@gmail.com>

Added tests for pathlike usage

acddf95

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero force-pushed the main branch from f10d10f to acddf95 Compare May 6, 2023 10:25

Merge branch 'main' into main

6f799bc

jcwchen reviewed May 8, 2023

View reviewed changes

Merged new ONNX version

5cfa29f

justinchuby reviewed May 13, 2023

View reviewed changes

onnx/test/basic_test.py Outdated Show resolved Hide resolved

justinchuby reviewed May 13, 2023

View reviewed changes

onnx/test/test_external_data.py Outdated Show resolved Hide resolved

tests should be compatible with new main

374a567

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero force-pushed the main branch from 5a2348b to 374a567 Compare May 14, 2023 16:57

vahvero requested a review from justinchuby May 14, 2023 17:57

vahvero and others added 3 commits May 15, 2023 19:30

Merge branch 'main' into main

d4c676d

Changed to newer typing system

a639f38

Signed-off-by: vahvero <otso.brummer@gmail.com>

Merge branch 'main' into main

0f60d4d

justinchuby reviewed May 15, 2023

View reviewed changes

onnx/__init__.py Outdated Show resolved Hide resolved

justinchuby reviewed May 15, 2023

View reviewed changes

onnx/checker.py Show resolved Hide resolved

justinchuby reviewed May 15, 2023

View reviewed changes

onnx/test/test_external_data.py Outdated Show resolved Hide resolved

vahvero and others added 4 commits May 16, 2023 18:00

Update onnx/__init__.py

6038225

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

Update onnx/checker.py

f5121cb

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

Update onnx/test/test_external_data.py

79d0529

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

Merge branch 'main' into main

7360a95

justinchuby reviewed May 16, 2023

View reviewed changes

onnx/__init__.py Outdated Show resolved Hide resolved

justinchuby reviewed May 16, 2023

View reviewed changes

onnx/utils.py Outdated Show resolved Hide resolved

vahvero and others added 3 commits May 19, 2023 13:31

Update onnx/__init__.py

7bc93db

Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com> Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

Merge branch 'main' into main

169aebc

Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

Update utils.py

0d99b0f

Signed-off-by: vahvero <30704280+vahvero@users.noreply.github.com>

justinchuby approved these changes May 19, 2023

View reviewed changes

jcwchen added the module: utility Helper modules label May 19, 2023

jcwchen approved these changes May 19, 2023

View reviewed changes

jcwchen added the run release CIs Use this label to trigger release tests in CI label May 19, 2023

Added pathlike to input types

495be78

Signed-off-by: vahvero <otso.brummer@gmail.com>

vahvero force-pushed the main branch from 27dfab1 to 495be78 Compare May 28, 2023 13:24

Merge branch 'main' into main

2de7bc1

jcwchen approved these changes May 28, 2023

View reviewed changes

jcwchen enabled auto-merge (squash) May 28, 2023 16:06

vahvero added 2 commits May 28, 2023 21:08

Removed unnesessary if-statement and made try-excepts more spesific.

e93e9a6

Signed-off-by: vahvero <otso.brummer@gmail.com>

Removed unnesessary if-statement and made try-excepts more spesific.

567a97d

Signed-off-by: vahvero <otso.brummer@gmail.com>

auto-merge was automatically disabled May 28, 2023 18:08
Head branch was pushed to by a user without write access

vahvero and others added 2 commits May 30, 2023 21:37

Merge branch 'main' into main

acd6c6a

Ran and fixed all linting errros

14b4b7c

Signed-off-by: vahvero <otso.brummer@gmail.com>

jcwchen merged commit ba9d006 into onnx:main May 30, 2023

	def infer_shapes_path(
	model_path: str,
	output_path: str = "",

Add os.PathLike when filepath as str is expected #5200

Add os.PathLike when filepath as str is expected #5200

Uh oh!

Conversation

vahvero commented May 4, 2023

Description

Motivation and Context

Context

Motivation

Uh oh!

justinchuby left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vahvero commented May 6, 2023

Uh oh!

jcwchen left a comment

Choose a reason for hiding this comment

Uh oh!

vahvero commented May 13, 2023

Uh oh!

Uh oh!

Uh oh!

justinchuby commented May 13, 2023

Uh oh!

vahvero commented May 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcwchen left a comment

Choose a reason for hiding this comment

Uh oh!

vahvero commented May 28, 2023

Uh oh!

justinchuby commented May 30, 2023

Uh oh!

vahvero commented May 30, 2023

Uh oh!

justinchuby commented May 30, 2023

Uh oh!

vahvero commented May 30, 2023

Uh oh!

Uh oh!

Add `os.PathLike` when filepath as `str` is expected #5200

Add `os.PathLike` when filepath as `str` is expected #5200

justinchuby left a comment •

edited

Loading