search

etsap-TIMES / xl2times

Open source tool to convert TIMES models specified in Excel
https://xl2times.readthedocs.io/
MIT License
12 stars 7 forks source link

CI: bring back subprocess to fix regression tests #197

Closed siddharth-krishna closed 7 months ago

siddharth-krishna commented 7 months ago

Now I remember why I used subprocess to call xl2times from run_benchmarks.py: in the CI, when it switches to the main branch, it needs to run the main branch's version of the tool. But if we call the tool as a python function, I think some of the PR version of the tool remains in memory, and we don't get what we want. This looks to be the reason CI is failing on this PR #183 : https://github.com/etsap-TIMES/xl2times/actions/runs/8020431584/job/21910275023

(The error is that it can't find a file in xl2times/config/... that was added by the PR when it is running tests in the main branch.)

This PR undoes the change from #193 that removed the use of subprocess. See also https://github.com/etsap-TIMES/xl2times/pull/193/files?diff=unified&w=0#r1498683705

olejandro commented 7 months ago

Thanks @siddharth-krishna! 👍

SamRWest commented 7 months ago

Interesting - did it fail to reload the main source code (and so was still looking for the new json file that didn't exist in main) in the non-subprocess version? If so there's probably importlib re-importing trickery that would fix that.

Another (possibly more robust, though slower) solution might be to clone main into a subdir, create a separate venv for it and run from there. This would have the advantage that you could run the comparisons locally even with a dirty working dir, and it'd still work if libraries had been removed etc (as the current mechanism uses the PR's libs to run main, which could be problematic).

And a third possibility: whenever CI runs on main, it creates a github release containing the benchmark results/table, then benchmarks could just download those to compare.

But anyway, it ain't broke at the moment, so probably low priority.

siddharth-krishna commented 7 months ago

Yes, I think it didn't reload the main source code. And nice ideas, thanks. I was actually thinking of trying (3) at some point, especially if CI starts to take longer, since right now we are redundantly running CI on main in every PR commit.