From accfe2f59e6dcc8e4be55133a2026d0b6d5b1234 Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Mon, 9 Jun 2025 10:57:24 -0500 Subject: [PATCH 1/2] First stab at requireing requirements --- rsconnect/environment.py | 41 ++++++++++---- rsconnect/subprocesses/inspect_environment.py | 55 ++++++++++++++----- tests/test_bundle.py | 10 ++-- tests/test_environment.py | 21 +++++-- .../test_subprocesses_inspect_environment.py | 47 ++++++++++++++++ 5 files changed, 139 insertions(+), 35 deletions(-) create mode 100644 tests/test_subprocesses_inspect_environment.py diff --git a/rsconnect/environment.py b/rsconnect/environment.py index 35280fef..5b57d5ad 100644 --- a/rsconnect/environment.py +++ b/rsconnect/environment.py @@ -101,6 +101,7 @@ def create_python_environment( python: typing.Optional[str] = None, override_python_version: typing.Optional[str] = None, app_file: typing.Optional[str] = None, + require_requirements_txt: bool = True, ) -> "Environment": """Given a project directory and a Python executable, return Environment information. @@ -122,7 +123,7 @@ def create_python_environment( # click.secho(' Deploying %s to server "%s"' % (directory, connect_server.url)) _warn_on_ignored_manifest(directory) - _warn_if_no_requirements_file(directory) + _check_requirements_file(directory, require_requirements_txt) _warn_if_environment_directory(directory) python_version_requirement = pyproject.detect_python_version_requirement(directory) @@ -145,7 +146,7 @@ def create_python_environment( python_version_requirement = f"=={override_python_version}" # with cli_feedback("Inspecting Python environment"): - environment = cls._get_python_env_info(module_file, python, force_generate) + environment = cls._get_python_env_info(module_file, python, force_generate, require_requirements_txt) environment.python_version_requirement = python_version_requirement if override_python_version: @@ -160,7 +161,7 @@ def create_python_environment( @classmethod def _get_python_env_info( - cls, file_name: str, python: typing.Optional[str], force_generate: bool = False + cls, file_name: str, python: typing.Optional[str], force_generate: bool = False, require_requirements_txt: bool = True ) -> "Environment": """ Gathers the python and environment information relating to the specified file @@ -175,7 +176,7 @@ def _get_python_env_info( """ python = which_python(python) logger.debug("Python: %s" % python) - environment = cls._inspect_environment(python, os.path.dirname(file_name), force_generate=force_generate) + environment = cls._inspect_environment(python, os.path.dirname(file_name), force_generate=force_generate, require_requirements_txt=require_requirements_txt) if environment.error: raise RSConnectException(environment.error) logger.debug("Python: %s" % python) @@ -189,6 +190,7 @@ def _inspect_environment( directory: str, force_generate: bool = False, check_output: typing.Callable[..., bytes] = subprocess.check_output, + require_requirements_txt: bool = True, ) -> "Environment": """Run the environment inspector using the specified python binary. @@ -202,7 +204,12 @@ def _inspect_environment( args = [python, "-m", "rsconnect.subprocesses.inspect_environment"] if flags: args.append("-" + "".join(flags)) + + # Add arguments for inspect_environment.py args.append(directory) + + if not require_requirements_txt: + args.append("--no-require-requirements") try: environment_json = check_output(args, text=True) @@ -293,19 +300,31 @@ def _warn_on_ignored_manifest(directory: str) -> None: ) -def _warn_if_no_requirements_file(directory: str) -> None: +def _check_requirements_file(directory: str, require_requirements_txt: bool = True) -> None: """ Checks for the existence of a file called requirements.txt in the given directory. - If it's not there, a warning will be printed. + Raises RSConnectException if it doesn't exist and require_requirements_txt is True. :param directory: the directory to check in. + :param require_requirements_txt: if True, raises exception when requirements.txt is missing. + if False, prints a warning instead (for backwards compatibility in tests). + :raises: RSConnectException if requirements.txt is missing and require_requirements_txt is True. """ if not os.path.exists(os.path.join(directory, "requirements.txt")): - click.secho( - " Warning: Capturing the environment using 'pip freeze'.\n" - " Consider creating a requirements.txt file instead.", - fg="yellow", - ) + if require_requirements_txt: + raise RSConnectException( + "requirements.txt file is required for deployment. \n" + "Please create a requirements.txt file in your project directory.\n" + "For best results, use a virtual environment and create your requirements.txt file with:\n" + "pip freeze > requirements.txt" + ) + else: + # For backwards compatibility in tests + click.secho( + " Warning: Capturing the environment using 'pip freeze'.\n" + " Consider creating a requirements.txt file instead.", + fg="yellow", + ) def _warn_if_environment_directory(directory: typing.Union[str, pathlib.Path]) -> None: diff --git a/rsconnect/subprocesses/inspect_environment.py b/rsconnect/subprocesses/inspect_environment.py index 97b2c9a4..80a060c3 100644 --- a/rsconnect/subprocesses/inspect_environment.py +++ b/rsconnect/subprocesses/inspect_environment.py @@ -60,13 +60,15 @@ class EnvironmentException(Exception): pass -def detect_environment(dirname: str, force_generate: bool = False) -> EnvironmentData: +def detect_environment(dirname: str, force_generate: bool = False, require_requirements_txt: bool = True) -> EnvironmentData: """Determine the python dependencies in the environment. - `pip freeze` will be used to introspect the environment. + `pip freeze` will be used to introspect the environment if force_generate=True or if + requirements.txt is missing and require_requirements_txt=False. :param: dirname Directory name :param: force_generate Force the generation of an environment + :param: require_requirements_txt If True, requirements.txt is required; otherwise pip freeze is used as fallback :return: a dictionary containing the package spec filename and contents if successful, or a dictionary containing `error` on failure. """ @@ -74,7 +76,15 @@ def detect_environment(dirname: str, force_generate: bool = False) -> Environmen if force_generate: result = pip_freeze() else: - result = output_file(dirname, "requirements.txt", "pip") or pip_freeze() + # Try to read requirements.txt file + try: + result = output_file(dirname, "requirements.txt", "pip") + except EnvironmentException as e: + # For backwards compatibility in tests + if not require_requirements_txt: + result = pip_freeze() + else: + raise e if result is not None: result["python"] = get_python_version() @@ -121,13 +131,13 @@ def output_file(dirname: str, filename: str, package_manager: str): """Read an existing package spec file. Returns a dictionary containing the filename and contents - if successful, None if the file does not exist, - or a dictionary containing 'error' on failure. + if successful, or raises an EnvironmentException if the file does not exist + or on other failures. """ try: path = os.path.join(dirname, filename) if not os.path.exists(path): - return None + raise EnvironmentException(f"{filename} file is required for deployment") with open(path, "r") as f: data = f.read() @@ -207,16 +217,35 @@ def main(): """ try: if len(sys.argv) < 2: - raise EnvironmentException("Usage: %s [-fc] DIRECTORY" % sys.argv[0]) - # directory is always the last argument - directory = sys.argv[len(sys.argv) - 1] + raise EnvironmentException("Usage: %s [-fc] DIRECTORY [--no-require-requirements]" % sys.argv[0]) + + # Parse arguments flags = "" force_generate = False - if len(sys.argv) > 2: + require_requirements_txt = True + + # Check for flags in first argument + if len(sys.argv) > 2 and sys.argv[1].startswith('-') and not sys.argv[1].startswith('--'): flags = sys.argv[1] - if "f" in flags: - force_generate = True - envinfo = detect_environment(directory, force_generate)._asdict() + if "f" in flags: + force_generate = True + + # Check for --no-require-requirements flag + if "--no-require-requirements" in sys.argv: + require_requirements_txt = False + + # directory is always the first non-flag argument + directory_index = 1 + while directory_index < len(sys.argv) and (sys.argv[directory_index].startswith('-') or + sys.argv[directory_index] == "--no-require-requirements"): + directory_index += 1 + + if directory_index >= len(sys.argv): + raise EnvironmentException("Missing directory argument") + + directory = sys.argv[directory_index] + + envinfo = detect_environment(directory, force_generate, require_requirements_txt)._asdict() if "contents" in envinfo: keepers = list(map(strip_ref, envinfo["contents"].split("\n"))) keepers = [line for line in keepers if not exclude(line)] diff --git a/tests/test_bundle.py b/tests/test_bundle.py index e300954e..2585aaac 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -79,7 +79,7 @@ def test_make_notebook_source_bundle1(self): # the test environment. Don't do this in the production code, which # runs in the notebook server. We need the introspection to run in # the kernel environment and not the notebook server environment. - environment = Environment.create_python_environment(directory) + environment = Environment.create_python_environment(directory, require_requirements_txt=False) with make_notebook_source_bundle( nb_path, environment, @@ -149,7 +149,7 @@ def test_make_notebook_source_bundle2(self): # the test environment. Don't do this in the production code, which # runs in the notebook server. We need the introspection to run in # the kernel environment and not the notebook server environment. - environment = Environment.create_python_environment(directory) + environment = Environment.create_python_environment(directory, require_requirements_txt=False) with make_notebook_source_bundle( nb_path, @@ -249,7 +249,7 @@ def test_make_quarto_source_bundle_from_simple_project(self): # input file. create_fake_quarto_rendered_output(temp_proj, "myquarto") - environment = Environment.create_python_environment(temp_proj) + environment = Environment.create_python_environment(temp_proj, require_requirements_txt=False) # mock the result of running of `quarto inspect ` inspect = { @@ -346,7 +346,7 @@ def test_make_quarto_source_bundle_from_complex_project(self): create_fake_quarto_rendered_output(site_dir, "index") create_fake_quarto_rendered_output(site_dir, "about") - environment = Environment.create_python_environment(temp_proj) + environment = Environment.create_python_environment(temp_proj, require_requirements_txt=False) # mock the result of running of `quarto inspect ` inspect = { @@ -448,7 +448,7 @@ def test_make_quarto_source_bundle_from_project_with_requirements(self): fp.write("pandas\n") fp.close() - environment = Environment.create_python_environment(temp_proj) + environment = Environment.create_python_environment(temp_proj, require_requirements_txt=False) # mock the result of running of `quarto inspect ` inspect = { diff --git a/tests/test_environment.py b/tests/test_environment.py index 1a1c5a95..75e8dff7 100644 --- a/tests/test_environment.py +++ b/tests/test_environment.py @@ -37,7 +37,7 @@ def test_get_default_locale(self): self.assertEqual(get_default_locale(lambda: (None, None)), "") def test_file(self): - result = Environment.create_python_environment(get_dir("pip1")) + result = Environment.create_python_environment(get_dir("pip1"), require_requirements_txt=True) self.assertTrue(version_re.match(result.pip)) @@ -59,7 +59,7 @@ def test_file(self): self.assertEqual(expected, result) def test_pip_freeze(self): - result = Environment.create_python_environment(get_dir("pip2")) + result = Environment.create_python_environment(get_dir("pip2"), require_requirements_txt=False) # these are the dependencies declared in our pyproject.toml self.assertIn("six", result.contents) @@ -83,6 +83,14 @@ def test_pip_freeze(self): python_interpreter=sys.executable, ) self.assertEqual(expected, result) + + def test_missing_requirements_file(self): + """Test that missing requirements.txt raises an exception""" + with tempfile.TemporaryDirectory() as empty_dir: + with self.assertRaises(RSConnectException) as context: + Environment.create_python_environment(empty_dir) + + self.assertIn("requirements.txt file is required", str(context.exception)) def test_filter_pip_freeze_output(self): raw_stdout = "numpy\npandas\n[notice] A new release of pip is available: 23.1.2 -> 23.3\n\ @@ -136,22 +144,22 @@ def test_is_not_executable(self): class TestPythonVersionRequirements: def test_pyproject_toml(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "using_pyproject")) + env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "using_pyproject"), require_requirements_txt=False) assert env.python_interpreter == sys.executable assert env.python_version_requirement == ">=3.8" def test_python_version(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "using_pyversion")) + env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "using_pyversion"), require_requirements_txt=False) assert env.python_interpreter == sys.executable assert env.python_version_requirement == ">=3.8,<3.12" def test_all_of_them(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "allofthem")) + env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "allofthem"), require_requirements_txt=False) assert env.python_interpreter == sys.executable assert env.python_version_requirement == ">=3.8,<3.12" def test_missing(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "empty")) + env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "empty"), require_requirements_txt=False) assert env.python_interpreter == sys.executable assert env.python_version_requirement is None @@ -256,6 +264,7 @@ def fake_inspect_environment( directory, force_generate=False, check_output=subprocess.check_output, + require_requirements_txt=True, ): return expected_environment diff --git a/tests/test_subprocesses_inspect_environment.py b/tests/test_subprocesses_inspect_environment.py new file mode 100644 index 00000000..a4622b2b --- /dev/null +++ b/tests/test_subprocesses_inspect_environment.py @@ -0,0 +1,47 @@ +import os +import tempfile +import pytest +from unittest import mock + +from rsconnect.exception import RSConnectException +from rsconnect.subprocesses.inspect_environment import ( + output_file, + detect_environment, + pip_freeze, + EnvironmentException, +) + + +def test_output_file_requires_requirements_txt(): + """Test that output_file raises an exception when requirements.txt is missing""" + with tempfile.TemporaryDirectory() as empty_dir: + with pytest.raises(EnvironmentException) as context: + output_file(empty_dir, "requirements.txt", "pip") + + assert "requirements.txt file is required" in str(context.value) + + +def test_detect_environment_requires_requirements_txt(): + """Test that detect_environment raises an exception when requirements.txt is missing""" + with tempfile.TemporaryDirectory() as empty_dir: + with pytest.raises(EnvironmentException) as context: + detect_environment(empty_dir, force_generate=False) + + assert "requirements.txt file is required" in str(context.value) + + +def test_detect_environment_with_force_generate(): + """Test that detect_environment still works with force_generate=True""" + with tempfile.TemporaryDirectory() as empty_dir: + with mock.patch('rsconnect.subprocesses.inspect_environment.pip_freeze') as mock_pip_freeze: + mock_pip_freeze.return_value = { + "filename": "requirements.txt", + "contents": "numpy\npandas", + "source": "pip_freeze", + "package_manager": "pip", + } + # This should not raise an exception + environment = detect_environment(empty_dir, force_generate=True) + assert environment.filename == "requirements.txt" + assert "numpy" in environment.contents + assert "pandas" in environment.contents From cbd2cf48a4fa9db8f44fecff23100d8f5d7a38a6 Mon Sep 17 00:00:00 2001 From: Jonathan Keane Date: Tue, 10 Jun 2025 07:25:54 -0500 Subject: [PATCH 2/2] linting --- rsconnect/environment.py | 17 +++++++++--- rsconnect/subprocesses/inspect_environment.py | 27 ++++++++++--------- tests/test_environment.py | 21 ++++++++++----- .../test_subprocesses_inspect_environment.py | 9 +++---- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/rsconnect/environment.py b/rsconnect/environment.py index 5b57d5ad..5e4de432 100644 --- a/rsconnect/environment.py +++ b/rsconnect/environment.py @@ -161,7 +161,11 @@ def create_python_environment( @classmethod def _get_python_env_info( - cls, file_name: str, python: typing.Optional[str], force_generate: bool = False, require_requirements_txt: bool = True + cls, + file_name: str, + python: typing.Optional[str], + force_generate: bool = False, + require_requirements_txt: bool = True, ) -> "Environment": """ Gathers the python and environment information relating to the specified file @@ -176,7 +180,12 @@ def _get_python_env_info( """ python = which_python(python) logger.debug("Python: %s" % python) - environment = cls._inspect_environment(python, os.path.dirname(file_name), force_generate=force_generate, require_requirements_txt=require_requirements_txt) + environment = cls._inspect_environment( + python, + os.path.dirname(file_name), + force_generate=force_generate, + require_requirements_txt=require_requirements_txt, + ) if environment.error: raise RSConnectException(environment.error) logger.debug("Python: %s" % python) @@ -204,10 +213,10 @@ def _inspect_environment( args = [python, "-m", "rsconnect.subprocesses.inspect_environment"] if flags: args.append("-" + "".join(flags)) - + # Add arguments for inspect_environment.py args.append(directory) - + if not require_requirements_txt: args.append("--no-require-requirements") diff --git a/rsconnect/subprocesses/inspect_environment.py b/rsconnect/subprocesses/inspect_environment.py index 80a060c3..4dab9c7b 100644 --- a/rsconnect/subprocesses/inspect_environment.py +++ b/rsconnect/subprocesses/inspect_environment.py @@ -60,10 +60,12 @@ class EnvironmentException(Exception): pass -def detect_environment(dirname: str, force_generate: bool = False, require_requirements_txt: bool = True) -> EnvironmentData: +def detect_environment( + dirname: str, force_generate: bool = False, require_requirements_txt: bool = True +) -> EnvironmentData: """Determine the python dependencies in the environment. - `pip freeze` will be used to introspect the environment if force_generate=True or if + `pip freeze` will be used to introspect the environment if force_generate=True or if requirements.txt is missing and require_requirements_txt=False. :param: dirname Directory name @@ -218,33 +220,34 @@ def main(): try: if len(sys.argv) < 2: raise EnvironmentException("Usage: %s [-fc] DIRECTORY [--no-require-requirements]" % sys.argv[0]) - + # Parse arguments flags = "" force_generate = False require_requirements_txt = True - + # Check for flags in first argument - if len(sys.argv) > 2 and sys.argv[1].startswith('-') and not sys.argv[1].startswith('--'): + if len(sys.argv) > 2 and sys.argv[1].startswith("-") and not sys.argv[1].startswith("--"): flags = sys.argv[1] if "f" in flags: force_generate = True - + # Check for --no-require-requirements flag if "--no-require-requirements" in sys.argv: require_requirements_txt = False - + # directory is always the first non-flag argument directory_index = 1 - while directory_index < len(sys.argv) and (sys.argv[directory_index].startswith('-') or - sys.argv[directory_index] == "--no-require-requirements"): + while directory_index < len(sys.argv) and ( + sys.argv[directory_index].startswith("-") or sys.argv[directory_index] == "--no-require-requirements" + ): directory_index += 1 - + if directory_index >= len(sys.argv): raise EnvironmentException("Missing directory argument") - + directory = sys.argv[directory_index] - + envinfo = detect_environment(directory, force_generate, require_requirements_txt)._asdict() if "contents" in envinfo: keepers = list(map(strip_ref, envinfo["contents"].split("\n"))) diff --git a/tests/test_environment.py b/tests/test_environment.py index 75e8dff7..c9a9700c 100644 --- a/tests/test_environment.py +++ b/tests/test_environment.py @@ -83,13 +83,13 @@ def test_pip_freeze(self): python_interpreter=sys.executable, ) self.assertEqual(expected, result) - + def test_missing_requirements_file(self): """Test that missing requirements.txt raises an exception""" with tempfile.TemporaryDirectory() as empty_dir: with self.assertRaises(RSConnectException) as context: Environment.create_python_environment(empty_dir) - + self.assertIn("requirements.txt file is required", str(context.exception)) def test_filter_pip_freeze_output(self): @@ -144,22 +144,30 @@ def test_is_not_executable(self): class TestPythonVersionRequirements: def test_pyproject_toml(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "using_pyproject"), require_requirements_txt=False) + env = Environment.create_python_environment( + os.path.join(TESTDATA, "python-project", "using_pyproject"), require_requirements_txt=False + ) assert env.python_interpreter == sys.executable assert env.python_version_requirement == ">=3.8" def test_python_version(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "using_pyversion"), require_requirements_txt=False) + env = Environment.create_python_environment( + os.path.join(TESTDATA, "python-project", "using_pyversion"), require_requirements_txt=False + ) assert env.python_interpreter == sys.executable assert env.python_version_requirement == ">=3.8,<3.12" def test_all_of_them(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "allofthem"), require_requirements_txt=False) + env = Environment.create_python_environment( + os.path.join(TESTDATA, "python-project", "allofthem"), require_requirements_txt=False + ) assert env.python_interpreter == sys.executable assert env.python_version_requirement == ">=3.8,<3.12" def test_missing(self): - env = Environment.create_python_environment(os.path.join(TESTDATA, "python-project", "empty"), require_requirements_txt=False) + env = Environment.create_python_environment( + os.path.join(TESTDATA, "python-project", "empty"), require_requirements_txt=False + ) assert env.python_interpreter == sys.executable assert env.python_version_requirement is None @@ -281,6 +289,7 @@ def fake_inspect_environment( assert environment.python_interpreter == expected_python assert environment == expected_environment + class TestEnvironmentDeprecations: def test_override_python_version(self): with mock.patch.object(rsconnect.environment.logger, "warning") as mock_warning: diff --git a/tests/test_subprocesses_inspect_environment.py b/tests/test_subprocesses_inspect_environment.py index a4622b2b..191ed42e 100644 --- a/tests/test_subprocesses_inspect_environment.py +++ b/tests/test_subprocesses_inspect_environment.py @@ -1,13 +1,10 @@ -import os import tempfile import pytest from unittest import mock -from rsconnect.exception import RSConnectException from rsconnect.subprocesses.inspect_environment import ( output_file, detect_environment, - pip_freeze, EnvironmentException, ) @@ -17,7 +14,7 @@ def test_output_file_requires_requirements_txt(): with tempfile.TemporaryDirectory() as empty_dir: with pytest.raises(EnvironmentException) as context: output_file(empty_dir, "requirements.txt", "pip") - + assert "requirements.txt file is required" in str(context.value) @@ -26,14 +23,14 @@ def test_detect_environment_requires_requirements_txt(): with tempfile.TemporaryDirectory() as empty_dir: with pytest.raises(EnvironmentException) as context: detect_environment(empty_dir, force_generate=False) - + assert "requirements.txt file is required" in str(context.value) def test_detect_environment_with_force_generate(): """Test that detect_environment still works with force_generate=True""" with tempfile.TemporaryDirectory() as empty_dir: - with mock.patch('rsconnect.subprocesses.inspect_environment.pip_freeze') as mock_pip_freeze: + with mock.patch("rsconnect.subprocesses.inspect_environment.pip_freeze") as mock_pip_freeze: mock_pip_freeze.return_value = { "filename": "requirements.txt", "contents": "numpy\npandas",