From dd6fc955b6bcac4416e38b787e12c2e29d164de3 Mon Sep 17 00:00:00 2001 From: Sam West Date: Thu, 15 Feb 2024 08:35:55 +1100 Subject: [PATCH] Addressed review comments --- .github/workflows/ci.yml | 5 ++--- .gitignore | 1 + .python-version | 1 - README.md | 5 +++-- pyproject.toml | 2 +- utils/__init__.py | 0 utils/run_benchmarks.py | 8 +++----- xl2times/__main__.py | 16 ++++++++++++++-- 8 files changed, 24 insertions(+), 14 deletions(-) delete mode 100644 .python-version create mode 100644 utils/__init__.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ff12b0b..7f42d5f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -110,8 +110,8 @@ jobs: GAMS_LICENSE: ${{ secrets.GAMS_LICENSE }} if: ${{ env.GAMS_LICENSE == '' }} working-directory: xl2times - # Run without --dd flag GAMS license secret doesn't exist to see if we're just missing a GAMS license. - # This way we still run CSV-only regression tests without GAMS - useful for testing in forks before creating PRs. + # Run without --dd flag if GAMS license secret doesn't exist. + # Useful for testing for (CSV) regressions in forks before creating PRs. run: | source .venv/bin/activate export PATH=$PATH:$GITHUB_WORKSPACE/GAMS/gams44.1_linux_x64_64_sfx @@ -120,7 +120,6 @@ jobs: --verbose \ | tee out.txt; \ echo ${PIPESTATUS[0]} > retcode.txt) - echo 'Note: Pipeline will fail in final step due to missing GAMS license' - name: Print summary working-directory: xl2times diff --git a/.gitignore b/.gitignore index 0dd196b..f71874c 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,4 @@ speedscope.json .venv*/ benchmarks/ .idea/ +.python-version diff --git a/.python-version b/.python-version deleted file mode 100644 index 0c7d5f5..0000000 --- a/.python-version +++ /dev/null @@ -1 +0,0 @@ -3.11.4 diff --git a/README.md b/README.md index b758cf3..bef1915 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ If you want to skip these pre-commit steps for a particular commit, if for insta git commit --no-verify ``` -### Publishing the Tool +## Publishing the Tool To publish a new version of the tool on PyPI, update the version number in `pyproject.toml`, and then run: ```bash @@ -87,6 +87,7 @@ Note that this assumes you have access to all these repositories (some are priva you'll have to request access) - if not, comment out the inaccessible benchmarks from `benchmakrs.yml` before running. ```bash +mkdir benchmarks # Get VEDA example models and reference DD files # XLSX files are in private repo for licensing reasons, please request access or replace with your own licensed VEDA example files. git clone git@github.com:olejandro/demos-xlsx.git benchmarks/xlsx/ @@ -99,7 +100,7 @@ git clone git@github.com:esma-cgep/tim-gams.git benchmarks/dd/Ireland Then to run the benchmarks: ```bash # Run a only a single benchmark by name (see benchmarks.yml for name list) -python utils/run_benchmarks.py benchmarks.yml --verbose --run DemoS_001-all +python utils/run_benchmarks.py benchmarks.yml --verbose --run DemoS_001-all | tee out.txt # Run all benchmarks (without GAMS run, just comparing CSV data) python utils/run_benchmarks.py benchmarks.yml --verbose | tee out.txt diff --git a/pyproject.toml b/pyproject.toml index a5dd4d6..5472df5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,7 +3,7 @@ requires = ["setuptools>=61.0.0", "wheel"] build-backend = "setuptools.build_meta" [tool.setuptools] -packages = ["xl2times"] +packages = ["xl2times", "utils"] [project] name = "xl2times" diff --git a/utils/__init__.py b/utils/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/utils/run_benchmarks.py b/utils/run_benchmarks.py index 9c58892..5ae8876 100644 --- a/utils/run_benchmarks.py +++ b/utils/run_benchmarks.py @@ -150,8 +150,6 @@ def run_benchmark( stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, - # windows needs this for subprocess to inherit venv when calling python directly: - shell=True if os.name == "nt" else False, ) if res.returncode != 0: # Remove partial outputs @@ -160,7 +158,7 @@ def run_benchmark( print(f"ERROR: dd_to_csv failed on {benchmark['name']}") sys.exit(5) else: - # subprocesses use too much RAM in windows, just use function call in current process instead + # If debug option is set, run as a function call to allow stepping with a debugger. from utils.dd_to_csv import main main([dd_folder, csv_folder]) @@ -203,14 +201,14 @@ def run_benchmark( runtime = time.time() - start - if verbose and res is not None: + if verbose: line = "-" * 80 print(f"\n{line}\n{benchmark['name']}\n{line}\n\n{res.stdout}") print(res.stderr if res.stderr is not None else "") else: print(".", end="", flush=True) - if res is not None and res.returncode != 0: + if res.returncode != 0: print(res.stdout) print(f"ERROR: tool failed on {benchmark['name']}") sys.exit(7) diff --git a/xl2times/__main__.py b/xl2times/__main__.py index a57a112..0e90ffe 100644 --- a/xl2times/__main__.py +++ b/xl2times/__main__.py @@ -387,6 +387,11 @@ def dump_tables(tables: List, filename: str) -> List: def run(args) -> str | None: + """ + Runs the xl2times conversion. + :param args: pre-parsed command line arguments + :return comparison with ground-truth string if `ground_truth_dir` is provided, else None. + """ config = datatypes.Config( "times_mapping.txt", "times-info.json", @@ -441,6 +446,10 @@ def run(args) -> str | None: def parse_args(arg_list: None | list[str]) -> argparse.Namespace: + """Parse command line arguments + :param arg_list: list of command line arguments. Uses sys.argv (default argpasrse behaviour) if `None`. + :return parsed arguments + """ args_parser = argparse.ArgumentParser() args_parser.add_argument( "input", @@ -478,11 +487,14 @@ def parse_args(arg_list: None | list[str]) -> argparse.Namespace: return args -def main(arg_list: None | list[str] = None): +def main(arg_list: None | list[str] = None) -> None: + """Main entry point for the xl2times package + :return: None. + """ args = parse_args(arg_list) run(args) if __name__ == "__main__": - main() + main(sys.argv[1:]) sys.exit(0)