Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance Enhancements #151

Open
wants to merge 76 commits into
base: development
Choose a base branch
from
Open

Performance Enhancements #151

wants to merge 76 commits into from

Conversation

Grochocinski
Copy link
Collaborator

@Grochocinski Grochocinski commented Mar 17, 2025

SimOpt Core

  • Fixed return typing for objectives and stoch_constraint stats
  • Fixed factor_settings_filename not being an optional argument for a DataFarmingExperiment
  • Changed multiprocessing from map_async to imap_unordered to reduce busy waiting and overall memory usage
  • Changed multiprocessing pool to use the min of (# CPU cores, # macroreps) to prevent spawning processes that will never be used or spawning more processes than can be handled at once
  • Fixed directory type hints to indicate subclasses of Problems/Solvers/Models are being returned rather than instances of the parent class

Problems/Models/Solvers

Testing

  • Refactored testing scripts with child/parent class layout to vastly reduce duplicate code and prevent large future refactorings
  • Introduced script to dynamically build test classes based on existing YAML files, allowing VS Code and unittest to automatically find the most up-to-date testing files
  • Updated expected results to align with prerelease mrg32k3a V1.1
  • Modified testing logic and base classes to prevent numpy's binary values from being dumped to expected YAMLs

Dev Utils

  • Generalized create_profiles.py into run_experiment.py to allow for easier runtime analysis with tools like VizTracer

Demo Files

  • Made simopt directory adding logic more robust
  • Formatted imports

Project Settings

  • Added imports flag ("I") to Ruff's linting
  • Added "simopt" and "mrg32k3a" to Ruff's known-first-party list to ensure import consistency regardless of mrg32k3a import method

@Grochocinski Grochocinski self-assigned this Mar 17, 2025
@Grochocinski
Copy link
Collaborator Author

Grochocinski commented Mar 17, 2025

Early testing with VizTracer seems to confirm that mrg32k3a random number generation is taking up a significant portion of processor time. It might be worth looking into further optimization such as JIT compilation or straight up reimplementing it in C/C++/Rust and creating Python bindings for that with PyO3.

EDIT: I made quite a few optimizations to the mrg32k3a and opened a PR. Testing shows a 30-40% speed increase which is probably the best we can get without implementing some JIT/reimplementation techniques. At What point do we hit diminishing returns on that?

@Grochocinski
Copy link
Collaborator Author

Grochocinski commented Mar 27, 2025

Current Performance Comparison vs Development

% Change in Runtime vs Development (Avg. -27.35%)

Problem ADAM ALOE ASTRODF NELDMD RNDSRCH SPSA STRONG
AMUSEMENTPARK-1 -19.64%
CHESS-1 -16.62%
CNTNEWS-1 -30.31% -33.55% -31.38% -32.50% -33.88% -32.20% -32.28%
CONTAM-1 -30.57%
CONTAM-2 -31.63%
DUALSOURCING-1 -79.42%
DYNAMNEWS-1 -32.59% -33.94% -25.60% -23.12% -35.35% -33.65% -35.39%
EXAMPLE-1 -32.07% -32.59% -35.52% -34.04% -34.45% -33.70% -34.36%
FACSIZE-1 -33.95%
FACSIZE-2 -33.81%
FIXEDSAN-1 17.51% 23.81% 67.20% 12.03% 2.50% 1.75% 14.64%
HOTEL-1 -8.25%
IRONORE-1 -32.28%
IRONORECONT-1 -30.79% -32.47% -27.72% -30.89% -31.54% -29.96% -30.71%
MM1-1 -6.95% -13.30% -10.97% -9.80% -19.98% -1.61% -14.95%
NETWORK-1 -21.19%
PARAMESTI-1 -33.43% -36.48% -36.11% -35.77% -34.62% -33.83% -35.78%
RMITD-1 -35.84%
SAN-1 -33.54% -36.58% -28.14% -37.39% -38.31% -38.10% -35.47%
SSCONT-1 -49.26% -49.04% -49.24% -46.56% -49.23% -50.00% -50.48%
TABLEALLOCATION-1 -35.68%

Testing Notes

  • Calculated as (new-old)/old (lower is better)
  • Values used is the total time to run, post-replicate, and post-normalize an experiment with 10 macroreps and 100 postreps

Notes on FIXEDSAN regression

As can be seen in the commit for the FIXEDSAN-1 refactor, the code was changed from a mess of calculations to a much simpler series of function calls and helper functions. This adds overhead, but in return allows much greater clarity into the code. This greater clarity even allowed for a bug that was present since the code's inception to be easily spotted (see #153).

If we ignore this regression, the average difference in speed is -32.21%

@Grochocinski Grochocinski requested a review from Copilot March 28, 2025 17:53
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces performance enhancements across core experiment functionality, testing scripts, demo files, and developer utilities. Key changes include improved return typing for objectives and constraints, refined multiprocessing pool usage to reduce overhead, and extensive refactoring of testing and demo code using pathlib for cleaner imports and path handling.

Reviewed Changes

Copilot reviewed 208 out of 208 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pyproject.toml Reformatted dependency and linting configuration for consistency.
docs/source/conf.py Minor formatting adjustment to support Sphinx documentation build.
dev_tools/run_experiment.py Added a new CLI experiment runner with improved compatibility checks.
dev_tools/profiling/create_profiles.py Removed legacy profiling script, likely replaced by run_experiment.py.
dev_tools/generate_experiment_results.py Refactored test generation using pathlib and streamlined file handling.
demo/* Updated import paths and object instantiation for clarity and robustness.
Comments suppressed due to low confidence (1)

demo/demo_problem.py:100

  • [nitpick] Consider adding an informative message to this assert statement (e.g., 'stoch_constraints should not be None') to clarify the failure condition.
assert mysolution.stoch_constraints is not None

@Grochocinski Grochocinski requested a review from Copilot March 29, 2025 07:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request introduces performance enhancements, refactors several modules for clarity and efficiency, and updates various demo and testing scripts. Key changes include:

  • Updates to dependency and linting configurations in pyproject.toml.
  • Reworking of experimental and testing scripts (run_experiment, generate_experiment_results) to improve runtime performance and maintainability.
  • Migration of demo files from os.path to pathlib for improved readability and consistency.

Reviewed Changes

Copilot reviewed 208 out of 208 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pyproject.toml Reformatted dependencies and updated linting configuration.
docs/source/conf.py Minor formatting adjustment for Sphinx configuration.
dev_tools/testing/init.py Removed docstring; init file now empty.
dev_tools/run_experiment.py New script to run experiments with multiprocessing improvements.
dev_tools/profiling/create_profiles.py Removed outdated profiling code.
dev_tools/generate_experiment_results.py Refactored test generation using pathlib and simplified compatibility checks.
demo/* (multiple demo files) Updated import paths using pathlib; small code adjustments and minor fixes.

@Grochocinski Grochocinski marked this pull request as ready for review March 29, 2025 07:15
@Grochocinski
Copy link
Collaborator Author

all the refactoring is done and the I've implemented fixes for the two issues I discovered during the rewrites, so I think we're good to review and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant