Copilot commented on code in PR #16707: URL: https://github.com/apache/iotdb/pull/16707#discussion_r2501647023
########## iotdb-core/ainode/build_binary.py: ########## @@ -0,0 +1,259 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + + +""" +PyInstaller build script (Python version) +""" + +import os +import shutil +import subprocess +import sys +from pathlib import Path + + +def setup_venv(): + """Create virtual environment if it doesn't exist""" + script_dir = Path(__file__).parent + venv_dir = script_dir / ".venv" + + if venv_dir.exists(): + print(f"Virtual environment already exists at: {venv_dir}") + return venv_dir + + print(f"Creating virtual environment at: {venv_dir}") + subprocess.run([sys.executable, "-m", "venv", str(venv_dir)], check=True) + print("Virtual environment created successfully") + return venv_dir + + +def get_venv_python(venv_dir): + """Get Python executable path in virtual environment""" + if sys.platform == "win32": + return venv_dir / "Scripts" / "python.exe" + else: + return venv_dir / "bin" / "python" + + +def update_pip(venv_python): + """ + Update pip in virtual environment. + + Note: subprocess.run() is synchronous and blocks until the subprocess completes. + This ensures pip upgrade finishes before the script continues. + """ + print("Updating pip...") + subprocess.run( + [str(venv_python), "-m", "pip", "install", "--upgrade", "pip"], check=True + ) + print("pip updated successfully") + + +def get_venv_env(venv_dir): + """Get environment variables for virtual environment activation""" + env = os.environ.copy() + venv_path = str(venv_dir.absolute()) + env["VIRTUAL_ENV"] = venv_path + + # Add venv bin directory to PATH + if sys.platform == "win32": + venv_bin = str(venv_dir / "Scripts") + else: + venv_bin = str(venv_dir / "bin") + + # Prepend venv bin to PATH to ensure venv tools take precedence + env["PATH"] = f"{venv_bin}{os.pathsep}{env.get('PATH', '')}" + + return env + + +def install_dependencies(venv_python, venv_dir, script_dir): + """ + Install dependencies using poetry. + + Note: subprocess.run() is synchronous and blocks until each command completes. + This ensures each step (poetry lock, install) finishes before proceeding. + + We configure poetry to use our .venv directory by: + 1. Configuring poetry to use in-project virtualenvs + 2. Setting poetry to use our .venv via poetry env use + 3. Running poetry lock and install which will use our .venv + """ + print("Installing dependencies with poetry...") + + # Get environment with VIRTUAL_ENV set + venv_env = get_venv_env(venv_dir) + + # Configure poetry to use in-project virtualenvs + # This makes poetry create/use .venv in the project directory + print("Configuring poetry to use in-project virtualenvs...") + try: + subprocess.run( + ["poetry", "config", "virtualenvs.in-project", "true"], + cwd=str(script_dir), + env=venv_env, + check=True, + capture_output=True, + text=True, + ) + except Exception: + pass # Configuration may already be set + + # Configure poetry to use our existing virtual environment + # This links poetry's management to our .venv directory + print(f"Configuring poetry to use virtual environment at: {venv_dir}") + result = subprocess.run( + ["poetry", "env", "use", str(venv_python)], + cwd=str(script_dir), + env=venv_env, + check=False, # Don't fail if venv is already configured + capture_output=True, + text=True, + ) + + # Check output - if poetry tries to recreate venv, that's okay as it will use our path + if result.stdout: + output = result.stdout.strip() + print(output) + if result.stderr: + stderr = result.stderr.strip() + # Ignore warnings about venv already existing or being created + if ( + "already been activated" not in stderr.lower() + and "already in use" not in stderr.lower() + ): + print(stderr) + + # Run poetry lock + print("Running poetry lock...") + result = subprocess.run( + ["poetry", "lock"], + cwd=str(script_dir), + env=venv_env, + check=True, + capture_output=True, + text=True, + ) + if result.stdout: + print(result.stdout) + if result.stderr: + print(result.stderr) + + # Run poetry install + # With VIRTUAL_ENV set and poetry env use configured, this should install into our .venv + print("Running poetry install...") + result = subprocess.run( + ["poetry", "install"], + cwd=str(script_dir), + env=venv_env, + check=True, + capture_output=True, + text=True, + ) + if result.stdout: + print(result.stdout) + if result.stderr: + print(result.stderr) + + print("Dependencies installed successfully") + + +def check_pyinstaller(venv_python): + """Check if PyInstaller is installed""" + try: + result = subprocess.run( + [ + str(venv_python), + "-c", + "import PyInstaller; print(PyInstaller.__version__)", + ], + capture_output=True, + text=True, + check=True, + ) + version = result.stdout.strip() + print(f"PyInstaller version: {version}") + return True + except (subprocess.CalledProcessError, FileNotFoundError): + print("Error: PyInstaller is not installed") + print("Please run: pip install pyinstaller") Review Comment: The error message states "Please run: pip install pyinstaller", but according to the pyproject.toml, pyinstaller is already included in the dependencies. If PyInstaller is missing at this point, it suggests the poetry install step failed. The error message should reflect this and suggest checking the poetry install output or running the dependency installation again. ```suggestion print("PyInstaller should be installed via Poetry. Please check the output of the dependency installation step (e.g., 'poetry install') for errors, and try running it again.") ``` ########## scripts/sbin/windows/start-ainode.bat: ########## @@ -26,54 +26,12 @@ echo ``````````````````````````` pushd %~dp0..\.. if NOT DEFINED IOTDB_AINODE_HOME set IOTDB_AINODE_HOME=%cd% -call %IOTDB_AINODE_HOME%\\conf\\windows\\ainode-env.bat %* -if %errorlevel% neq 0 ( - echo Environment check failed. Exiting... - exit /b 1 -) +set $ain_ainode_executable=%IOTDB_AINODE_HOME%\\lib\\ainode\\ainode -for /f "tokens=2 delims==" %%a in ('findstr /i /c:"^ain_interpreter_dir" "%IOTDB_AINODE_HOME%\\conf\\windows\\ainode-env.bat"') do ( - set _ain_interpreter_dir=%%a - goto :done -) - -:initial -if "%1"=="" goto done -set aux=%1 -if "%aux:~0,1%"=="-" ( - set nome=%aux:~1,250% -) else ( - set "%nome%=%1" - set nome= -) -shift -goto initial - -:done -if "%i%"=="" ( - if "%_ain_interpreter_dir%"=="" ( - set _ain_interpreter_dir=%IOTDB_AINODE_HOME%\\venv\\Scripts\\python.exe - ) -) else ( - set _ain_interpreter_dir=%i% -) - -echo Script got parameter: ain_interpreter_dir: %_ain_interpreter_dir% - -cd %IOTDB_AINODE_HOME% - -for %%i in ("%_ain_interpreter_dir%") do set "parent=%%~dpi" - -set ain_ainode_dir=%parent%\ainode.exe - -set ain_ainode_dir_new=%parent%\Scripts\\ainode.exe +echo Script got ainode executable: "$ain_ainode_executable" echo Starting AINode... %ain_ainode_dir% start Review Comment: The variable `%ain_ainode_dir%` is no longer defined in this script. Based on line 29, this should be `%ain_ainode_executable%` instead. Additionally, the variable definition on line 29 has a syntax error that needs to be fixed first. ########## iotdb-core/ainode/ainode.xml: ########## @@ -54,17 +58,6 @@ <fileSet> <directory>dist</directory> Review Comment: The `dist` directory is now being packaged into `lib` without any file type filtering (previously it only included `*.whl` files). Since PyInstaller creates a directory structure in `dist/ainode/`, this change appears correct. However, verify that the entire `dist` directory should be included or if it should specifically target `dist/ainode/` to avoid packaging any build artifacts that might be in `dist/`. ```suggestion <directory>dist/ainode</directory> ``` ########## iotdb-core/ainode/iotdb/ainode/core/inference/pool_controller.py: ########## @@ -82,24 +82,25 @@ def first_req_init(self, model_id: str): """ Initialize the pools when the first request for the given model_id arrives. """ - if not self.has_request_pools(model_id, device.index): - # TODO: choose a device based on some strategy - device = self.DEFAULT_DEVICE - actions = self._pool_scheduler.schedule(model_id, device) - for action in actions: - if action.action == ScaleActionType.SCALE_UP: - # initialize the first pool - self._first_pool_init(action.model_id, str(device)) - # start a background thread to expand pools - expand_thread = threading.Thread( - target=self._expand_pools_on_device, - args=(action.model_id, str(device), action.amount - 1), - daemon=True, - ) - expand_thread.start() - elif action.action == ScaleActionType.SCALE_DOWN: - # TODO: implement scale down logic - pass + pass + # if not self.has_request_pools(model_id, device.index): + # # TODO: choose a device based on some strategy + # device = self.DEFAULT_DEVICE + # actions = self._pool_scheduler.schedule(model_id, device) + # for action in actions: + # if action.action == ScaleActionType.SCALE_UP: + # # initialize the first pool + # self._first_pool_init(action.model_id, str(device)) + # # start a background thread to expand pools + # expand_thread = threading.Thread( + # target=self._expand_pools_on_device, + # args=(action.model_id, str(device), action.amount - 1), + # daemon=True, + # ) + # expand_thread.start() + # elif action.action == ScaleActionType.SCALE_DOWN: + # # TODO: implement scale down logic + # pass Review Comment: This code has been commented out but left in the file. If this functionality is no longer needed (which appears to be the case given the PR's migration to PyInstaller), this commented code should be removed to improve code maintainability and reduce confusion. ```suggestion ``` ########## iotdb-core/ainode/build_binary.py: ########## @@ -0,0 +1,259 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + + +""" +PyInstaller build script (Python version) +""" + +import os +import shutil +import subprocess +import sys +from pathlib import Path + + +def setup_venv(): + """Create virtual environment if it doesn't exist""" + script_dir = Path(__file__).parent + venv_dir = script_dir / ".venv" + + if venv_dir.exists(): + print(f"Virtual environment already exists at: {venv_dir}") + return venv_dir + + print(f"Creating virtual environment at: {venv_dir}") + subprocess.run([sys.executable, "-m", "venv", str(venv_dir)], check=True) + print("Virtual environment created successfully") + return venv_dir + + +def get_venv_python(venv_dir): + """Get Python executable path in virtual environment""" + if sys.platform == "win32": + return venv_dir / "Scripts" / "python.exe" + else: + return venv_dir / "bin" / "python" + + +def update_pip(venv_python): + """ + Update pip in virtual environment. + + Note: subprocess.run() is synchronous and blocks until the subprocess completes. + This ensures pip upgrade finishes before the script continues. + """ + print("Updating pip...") + subprocess.run( + [str(venv_python), "-m", "pip", "install", "--upgrade", "pip"], check=True + ) + print("pip updated successfully") + + +def get_venv_env(venv_dir): + """Get environment variables for virtual environment activation""" + env = os.environ.copy() + venv_path = str(venv_dir.absolute()) + env["VIRTUAL_ENV"] = venv_path + + # Add venv bin directory to PATH + if sys.platform == "win32": + venv_bin = str(venv_dir / "Scripts") + else: + venv_bin = str(venv_dir / "bin") + + # Prepend venv bin to PATH to ensure venv tools take precedence + env["PATH"] = f"{venv_bin}{os.pathsep}{env.get('PATH', '')}" + + return env + + +def install_dependencies(venv_python, venv_dir, script_dir): + """ + Install dependencies using poetry. + + Note: subprocess.run() is synchronous and blocks until each command completes. + This ensures each step (poetry lock, install) finishes before proceeding. + + We configure poetry to use our .venv directory by: + 1. Configuring poetry to use in-project virtualenvs + 2. Setting poetry to use our .venv via poetry env use + 3. Running poetry lock and install which will use our .venv + """ + print("Installing dependencies with poetry...") + + # Get environment with VIRTUAL_ENV set + venv_env = get_venv_env(venv_dir) + + # Configure poetry to use in-project virtualenvs + # This makes poetry create/use .venv in the project directory + print("Configuring poetry to use in-project virtualenvs...") + try: + subprocess.run( + ["poetry", "config", "virtualenvs.in-project", "true"], + cwd=str(script_dir), + env=venv_env, + check=True, + capture_output=True, + text=True, + ) + except Exception: + pass # Configuration may already be set + Review Comment: The comment states "subprocess.run() is synchronous and blocks until the subprocess completes" which is accurate. However, using `capture_output=True` without checking the return value or handling potential errors could silently fail. Consider adding error handling or at least logging when poetry configuration fails. ```suggestion except Exception as e: print(f"Warning: poetry configuration failed: {e}") # If the subprocess raised CalledProcessError, it may have output if hasattr(e, 'stdout') and e.stdout: print("poetry config stdout:", e.stdout.strip()) if hasattr(e, 'stderr') and e.stderr: print("poetry config stderr:", e.stderr.strip()) print("Continuing; poetry configuration may already be set.") ``` ########## scripts/sbin/windows/start-ainode.bat: ########## @@ -26,54 +26,12 @@ echo ``````````````````````````` pushd %~dp0..\.. if NOT DEFINED IOTDB_AINODE_HOME set IOTDB_AINODE_HOME=%cd% -call %IOTDB_AINODE_HOME%\\conf\\windows\\ainode-env.bat %* -if %errorlevel% neq 0 ( - echo Environment check failed. Exiting... - exit /b 1 -) +set $ain_ainode_executable=%IOTDB_AINODE_HOME%\\lib\\ainode\\ainode -for /f "tokens=2 delims==" %%a in ('findstr /i /c:"^ain_interpreter_dir" "%IOTDB_AINODE_HOME%\\conf\\windows\\ainode-env.bat"') do ( - set _ain_interpreter_dir=%%a - goto :done -) - -:initial -if "%1"=="" goto done -set aux=%1 -if "%aux:~0,1%"=="-" ( - set nome=%aux:~1,250% -) else ( - set "%nome%=%1" - set nome= -) -shift -goto initial - -:done -if "%i%"=="" ( - if "%_ain_interpreter_dir%"=="" ( - set _ain_interpreter_dir=%IOTDB_AINODE_HOME%\\venv\\Scripts\\python.exe - ) -) else ( - set _ain_interpreter_dir=%i% -) - -echo Script got parameter: ain_interpreter_dir: %_ain_interpreter_dir% - -cd %IOTDB_AINODE_HOME% - -for %%i in ("%_ain_interpreter_dir%") do set "parent=%%~dpi" - -set ain_ainode_dir=%parent%\ainode.exe - -set ain_ainode_dir_new=%parent%\Scripts\\ainode.exe +echo Script got ainode executable: "$ain_ainode_executable" Review Comment: The variable reference uses incorrect syntax. In batch files, the dollar sign should not be used in variable references. This should be: ```batch echo Script got ainode executable: "%ain_ainode_executable%" ``` ########## scripts/sbin/windows/start-ainode.bat: ########## @@ -26,54 +26,12 @@ echo ``````````````````````````` pushd %~dp0..\.. if NOT DEFINED IOTDB_AINODE_HOME set IOTDB_AINODE_HOME=%cd% -call %IOTDB_AINODE_HOME%\\conf\\windows\\ainode-env.bat %* -if %errorlevel% neq 0 ( - echo Environment check failed. Exiting... - exit /b 1 -) +set $ain_ainode_executable=%IOTDB_AINODE_HOME%\\lib\\ainode\\ainode Review Comment: The variable name `$ain_ainode_executable` uses a dollar sign prefix which is incorrect for Windows batch files. In batch files, variables should be defined without `$` and referenced with `%variable%` syntax. This line should be: ```batch set ain_ainode_executable=%IOTDB_AINODE_HOME%\lib\ainode\ainode ``` ########## iotdb-core/ainode/ainode.spec: ########## @@ -0,0 +1,194 @@ +# -*- mode: python ; coding: utf-8 -*- +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under this License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +from pathlib import Path + +# Get project root directory +project_root = Path(SPECPATH).parent + +block_cipher = None + +# Auto-collect all submodules of large dependency libraries +# Using collect_all automatically includes all dependencies and avoids manual maintenance of hiddenimports +from PyInstaller.utils.hooks import collect_all, collect_submodules, collect_data_files + +# Collect only essential data files and binaries for large libraries +# Using collect_all for all submodules slows down startup significantly +# Instead, we collect only what's needed and rely on PyInstaller's dependency analysis +all_datas = [] +all_binaries = [] +all_hiddenimports = [] + +# Only collect essential data files and binaries for critical libraries +# This reduces startup time by avoiding unnecessary module imports +essential_libraries = { + 'torch': True, # Keep collect_all for torch as it has many dynamic imports + 'transformers': True, # Keep collect_all for transformers + 'safetensors': True, # Keep collect_all for safetensors +} + +# For other libraries, use selective collection to speed up startup +other_libraries = ['sktime', 'scipy', 'pandas', 'sklearn', 'statsmodels', 'optuna'] + +for lib in essential_libraries: + try: + lib_datas, lib_binaries, lib_hiddenimports = collect_all(lib) + all_datas.extend(lib_datas) + all_binaries.extend(lib_binaries) + all_hiddenimports.extend(lib_hiddenimports) + except Exception: + pass + +# For other libraries, only collect submodules (lighter weight) +# This relies on PyInstaller's dependency analysis to include what's actually used +for lib in other_libraries: + try: + submodules = collect_submodules(lib) + all_hiddenimports.extend(submodules) + # Only collect essential data files and binaries, not all submodules + # This significantly reduces startup time + try: + lib_datas, lib_binaries, _ = collect_all(lib) + all_datas.extend(lib_datas) + all_binaries.extend(lib_binaries) + except Exception: + # If collect_all fails, try collect_data_files for essential data only + try: + lib_datas = collect_data_files(lib) + all_datas.extend(lib_datas) + except Exception: + pass + except Exception: + pass + +# Project-specific packages that need their submodules collected +# Only list top-level packages - collect_submodules will recursively collect all submodules +TOP_LEVEL_PACKAGES = [ + 'iotdb.ainode.core', # This will include all sub-packages: manager, model, inference, etc. + 'iotdb.thrift', # This will include all thrift sub-packages +] + +# Collect all submodules for project packages automatically +# Using top-level packages avoids duplicate collection +for package in TOP_LEVEL_PACKAGES: + try: + submodules = collect_submodules(package) + all_hiddenimports.extend(submodules) + except Exception: + # If package doesn't exist or collection fails, add the package itself + all_hiddenimports.append(package) + +# Add parent packages to ensure they are included +all_hiddenimports.extend(['iotdb', 'iotdb.ainode']) + +# Multiprocessing support for PyInstaller +# When using multiprocessing with PyInstaller, we need to ensure proper handling +multiprocessing_modules = [ + 'multiprocessing', + 'multiprocessing.spawn', + 'multiprocessing.popen_spawn_posix', + 'multiprocessing.popen_spawn_win32', + 'multiprocessing.popen_fork', + 'multiprocessing.popen_forkserver', + 'multiprocessing.context', + 'multiprocessing.reduction', + 'multiprocessing.util', + 'torch.multiprocessing', + 'torch.multiprocessing.spawn', +] + +# Additional dependencies that may need explicit import +# These are external libraries that might use dynamic imports +external_dependencies = [ + 'huggingface_hub', + 'tokenizers', + 'hf_xet', + 'einops', + 'dynaconf', + 'tzlocal', + 'thrift', + 'psutil', + 'requests', +] + +all_hiddenimports.extend(multiprocessing_modules) +all_hiddenimports.extend(external_dependencies) + +# Analyze main entry file +a = Analysis( + ['iotdb/ainode/core/script.py'], + pathex=[str(project_root)], + binaries=all_binaries, + datas=all_datas, + hiddenimports=all_hiddenimports, + hookspath=[], + hooksconfig={}, + runtime_hooks=[], + excludes=[ + # Exclude unnecessary modules to reduce size and improve startup time + # Note: Do not exclude unittest, as torch and other libraries require it + # Only exclude modules that are definitely not used and not required by dependencies Review Comment: The comment on line 146 states "Do not exclude unittest, as torch and other libraries require it" but this exclusion list doesn't include `unittest`, so the comment is somewhat misleading. Consider rephrasing to: "Note: unittest is not excluded as torch and other libraries require it. Only exclude modules that are definitely not used and not required by dependencies." ```suggestion # Note: unittest is not excluded, as torch and other libraries require it. # Only exclude modules that are definitely not used and not required by dependencies. ``` ########## iotdb-core/ainode/ainode.spec: ########## @@ -0,0 +1,194 @@ +# -*- mode: python ; coding: utf-8 -*- +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under this License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +from pathlib import Path + +# Get project root directory +project_root = Path(SPECPATH).parent + +block_cipher = None + +# Auto-collect all submodules of large dependency libraries +# Using collect_all automatically includes all dependencies and avoids manual maintenance of hiddenimports +from PyInstaller.utils.hooks import collect_all, collect_submodules, collect_data_files + +# Collect only essential data files and binaries for large libraries +# Using collect_all for all submodules slows down startup significantly +# Instead, we collect only what's needed and rely on PyInstaller's dependency analysis Review Comment: The comment states "Using collect_all for all submodules slows down startup significantly" but then immediately uses `collect_all` for essential libraries. This is somewhat contradictory. Consider clarifying that collect_all is slow but necessary for certain libraries with dynamic imports (torch, transformers, safetensors), while other libraries can use lighter-weight collection methods. ```suggestion # Using collect_all for all submodules slows down startup significantly. # However, for certain libraries with many dynamic imports (e.g., torch, transformers, safetensors), # collect_all is necessary to ensure all required modules are included. # For other libraries, we use lighter-weight collection methods to improve startup time. ``` ########## iotdb-core/ainode/iotdb/ainode/core/script.py: ########## @@ -74,6 +75,14 @@ def remove_ainode(arguments): def main(): + # Handle PyInstaller: filter out Python arguments that might be passed to subprocesses + # These arguments are not needed in frozen executables and cause warnings + # Note: This filtering should happen AFTER freeze_support() has handled child processes + if getattr(sys, "frozen", False): + python_args_to_filter = ["-I", "-B", "-S", "-E", "-O", "-OO"] + sys.argv = [arg for arg in sys.argv if arg not in python_args_to_filter] Review Comment: The comment states "This filtering should happen AFTER freeze_support() has handled child processes" but the filtering actually happens BEFORE the freeze_support() calls (which are at lines 123-125, inside the `if __name__ == "__main__"` block). This creates a potential issue: if this is a child process spawned by multiprocessing, it will execute this filtering code before freeze_support() can properly handle it. Consider moving this filtering logic after the freeze_support() calls, or clarify why it needs to happen in main() rather than in the `if __name__ == "__main__"` block. ########## iotdb-core/ainode/iotdb/ainode/core/inference/pool_controller.py: ########## @@ -82,24 +82,25 @@ def first_req_init(self, model_id: str): """ Initialize the pools when the first request for the given model_id arrives. """ - if not self.has_request_pools(model_id, device.index): - # TODO: choose a device based on some strategy - device = self.DEFAULT_DEVICE - actions = self._pool_scheduler.schedule(model_id, device) - for action in actions: - if action.action == ScaleActionType.SCALE_UP: - # initialize the first pool - self._first_pool_init(action.model_id, str(device)) - # start a background thread to expand pools - expand_thread = threading.Thread( - target=self._expand_pools_on_device, - args=(action.model_id, str(device), action.amount - 1), - daemon=True, - ) - expand_thread.start() - elif action.action == ScaleActionType.SCALE_DOWN: - # TODO: implement scale down logic - pass + pass + # if not self.has_request_pools(model_id, device.index): + # # TODO: choose a device based on some strategy + # device = self.DEFAULT_DEVICE + # actions = self._pool_scheduler.schedule(model_id, device) + # for action in actions: + # if action.action == ScaleActionType.SCALE_UP: + # # initialize the first pool + # self._first_pool_init(action.model_id, str(device)) + # # start a background thread to expand pools + # expand_thread = threading.Thread( + # target=self._expand_pools_on_device, + # args=(action.model_id, str(device), action.amount - 1), + # daemon=True, + # ) + # expand_thread.start() + # elif action.action == ScaleActionType.SCALE_DOWN: + # # TODO: implement scale down logic + # pass Review Comment: This comment appears to contain commented-out code. ```suggestion ``` ########## iotdb-core/ainode/iotdb/ainode/core/inference/pool_controller.py: ########## @@ -277,7 +278,7 @@ def _expand_pool_on_device(*_): ) pool.start() self._register_pool(model_id, device_id, pool_id, pool, result_queue) Review Comment: The timeout has been increased from 30 seconds to 300 seconds (5 minutes). While this may be intentional for the PyInstaller-based approach, this is a significant change that could mask underlying issues. Consider if this timeout increase is necessary, and if so, add a comment explaining why the frozen executable requires a longer initialization time. ```suggestion self._register_pool(model_id, device_id, pool_id, pool, result_queue) # The timeout for pool.ready_event.wait() is set to 300 seconds (5 minutes) to accommodate # potentially slow initialization, such as loading large models or allocating hardware resources. # This is especially important when running as a frozen executable (e.g., via PyInstaller), # where startup overhead can be significant. If initialization consistently takes longer, # consider investigating underlying issues. ``` ########## iotdb-core/ainode/build_binary.py: ########## @@ -0,0 +1,259 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + + +""" +PyInstaller build script (Python version) +""" + +import os +import shutil Review Comment: Import of 'shutil' is not used. ```suggestion ``` -- 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]
