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

newexp wrapper updates #258

Merged
merged 7 commits into from
Sep 29, 2017
Merged

newexp wrapper updates #258

merged 7 commits into from
Sep 29, 2017

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Sep 29, 2017

This PR updates the wrap-fastframe and wrap-newexp MPI wrappers as well as fixing several bugs discovered while re-establishing the full end-to-end simulation + processing chain.

An API change is that desisim.simspec.simulate_spectra() now uses 1e-17 erg/s/cm2/A as default units instead of without 1e-17 (though you can still pass it a unit-ful astropy Quantity object and it will use that).

This has been tested with the minitest branch of https://github.com/desihub/two_percent_DESI .

Comments / suggestions are welcome, but I'd like to move forward with a fresh set of tags so comments sooner than later would be much appreciated.

@sbailey
Copy link
Contributor Author

sbailey commented Sep 29, 2017

I'm going to merge this in order to include it with today's set of self-consistent tags, but retroactive comments are still welcome and can be included later.

@sbailey sbailey merged commit 0791cac into master Sep 29, 2017
@sbailey sbailey deleted the newexp branch September 29, 2017 23:36
@moustakas
Copy link
Member

Thanks for this PR @sbailey. @akremin does this fix the "memory leak" problem you were having previously? And this looks like it'll address the "update database" issue I was seeing. I'll try to test the code soon and will report back --

@sbailey
Copy link
Contributor Author

sbailey commented Oct 16, 2017

This PR has wrap-newexp call newexp by spawning a system call to the script instead of calling desisim.scripts.newexp.main() as a function call. That is a pragmatic workaround to the memory leak problem, but not a very satisfying one since we still don't understand why it was leaking in the first place.

Other info:

  • For a couple hours one day at NERSC, subprocess.call() was randomly and immediately failing on some calls. Then the problem suddenly went away, even from within the same interactive batch job that was previously failing. NERSC wasn't able to track the problem to any other known issues (network problems, I/O, node hot swaps...) and we haven't been able to reproduce it since. But it may not be the most robust solution.
  • One of the original motivations for originally calling desisim.scripts.newexp.main() was to avoid the extra python startup overhead from repeatly rerunning python, triggering numpy imports etc. That hasn't been an issue recently, and if we do have to run at large scale we may be using docker containers anyway where re-spawning python commands is much less of an issue.

i.e. this works for now, it isn't outright "wrong", but it is a bit dissatisfying that the function calls were problematic due to a memory leak that we don't understand.

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.

2 participants