-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
Scatter parameters once in TPI #925
Conversation
@talumbau. I ran all the GH CI tests plus the ones marked as local, and I got a ton of the same errors as the one error in the GH CI In any event, below is my run of the full testing suite locally. You can ignore the three failures in
|
The great news is that there are no |
Oops. Too much copy/paste. The solution has to be a bit different here because we don't actually scatter inside the inner_loop, as was done in SS. I'll fix it up and run tests locally before pushing again. |
- Similar to the change in SS, put the parameters in global scope, then in `inner_loop` retrieve if possible. If they are not present, scatter them once for all future execution of `inner_loop`.
@talumbau. Just a warning that the full battery of tests running |
OK just finishing running the example here. The good news is that I think this change gets rid of all of the warnings about excessive garbage collection from Dask. However, I noticed that TPI is taking a long time for me and it looks like one process is doing a lot of work and many of the others are doing almost nothing. See this screenshot of top during the run: so it looks like one of the "categories" is doing more computational work than the others. |
I fixed the issue and pushed the changes to my branch. Running test_TPI.py right now. Should be done by tomorrow. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #925 +/- ##
==========================================
+ Coverage 73.43% 73.44% +0.01%
==========================================
Files 19 19
Lines 4641 4643 +2
==========================================
+ Hits 3408 3410 +2
Misses 1233 1233
Flags with carried forward coverage won't be shown. Click here to find out more.
|
OK, I confirm that
So, now the most helpful thing would be for someone else to do an A/B comparison for some of these tests (or maybe just
So around 12 hours on runtime. But I don't believe that my longer runtimes have anything to do with these dask optimizations I'm doing. So, @rickecon and @jdebacker can you do an A/B comparison of just the |
@talumbau I currently have all the OG-Core tests running on my machine. I started them last night, so they should finish in the next hour. Then I will run and time, Will you please make the following additions to this PR.
## [0.11.6] - 2024-04-17 14:00:00
### Added
- Scatters parameters once in `TPI.py`
[0.11.6]: https://github.com/PSLmodels/OG-Core/compare/v0.11.5...v0.11.6 |
@talumbau @jdebacker. Full set of tests looks great. Only tests failing locally are those three
|
@talumbau. I just submitted a PR to your branch that has the version updates to |
Rick's run of In summary, the baseline TPI takes 1 hour and 34 minutes, and the reform TPI takes 1 hour and 21 minutes. I created a branch off my updated OG-USA master branch, and added an Baseline steady state
Baseline transition path (1 hour, 34 min, 22.8 sec)
Reform steady-state
Reform transition path (1 hour, 21 min, 2.3 sec)
|
Rick's run of In summary, the baseline TPI takes 1 hour and 43 minutes, and the reform TPI takes 1 hour and 36 minutes. I created fresh Baseline steady state
Baseline transition path (1 hour, 42 min, 59.1 sec)
Reform steady-state
Reform transition path (1 hour, 35 min, 33.8 sec)
|
@talumbau and @jdebacker. Here is a summary of my two sets of OG-USA runs on the old OG-Core (v.0.11.5) and on the new OG-Core (v.0.11.6).
|
Great summary @rickecon! I ran the baseline on this branch. Baseline TPI took 49 minutes. I had a few garbage collection warnings in TPI (none in SS), but there seemed to be fewer than on OG-Core 0.11.5. But I will try to do the reform run in the next day or two -- seems there were more issues with that? |
@jdebacker. I don't see any issues with this PR. I want to merge it in and get that new version of OG-Core up. Just waiting for those last few commits to go in. |
@jdebacker. Your 2x faster runtime is because you have the Anaconda distribution for Apple M1. I haven't tested my machine in a year, but I should check now if my one other package now runs on that software. |
This is great info, thanks so much! It's the right answer to move this scatter as I'm doing in this PR, but it doesn't move the needle much on runtime. Something really strange is happening with my runtimes - it's literally taking me 10 times longer to run my code on a 128 core AMD Ryzen Threadripper machine with 256GB of RAM. I have a MacBook Air that's a few years old. Maybe I can try to run on that to see if it will actually do better than this Linux workstation. So next steps would be:
@rickecon can you post info on the machine you ran on? I'm assuming it's some kind of MacBookPro? CPU, RAM, MacOS version, etc. |
Updated version and removed Python 3.9 tests
Thanks so much for this update @talumbau. That is a big step forward for the project to have some better memory management. Merging as soon as all the tests pass (I updated the date and time of version update in |
inner_loop
retrieve if possible. If they are not present, scatter them once for all future execution ofinner_loop
.