search

MolSSI / QCEngine

Quantum chemistry program executor and IO standardizer (QCSchema).
https://molssi.github.io/QCEngine/
BSD 3-Clause "New" or "Revised" License
163 stars 79 forks source link

Make QCEngine tolerant of `cpuinfo` failure to populate `brand_raw`, `brand` #339

Closed dotsdl closed 2 years ago

dotsdl commented 2 years ago

Description

Currently, we observe frequent failure of cpuinfo to give brand information when used within Docker containers on Pacific Research Platform. It is unclear why, but making QCEngine tolerant of this information being missing would help in production usage.

Changelog description

QCEngine calls now tolerant of missing brand_raw or brand info from py-cpuinfo.

Status

codecov[bot] commented 2 years ago

Codecov Report

Merging #339 (e41cf50) into master (43b0713) will not change coverage. The diff coverage is 0.00%.

dotsdl commented 2 years ago

Thanks @WardLT for the review! I've made the changes you requested. Let me know if there's anything else I can do to move this forward!

dotsdl commented 2 years ago

Looks like the remaining CI errors may be unrelated to the changes in this PR. @loriab is this consistent with your view?

loriab commented 2 years ago

Agreed, if you make this change https://github.com/MolSSI/QCEngine/pull/338/files#diff-d66842b418a1c03f41438563803c987c9defd1811ea0af131e285ba540c8c0d6R208, it'll fix it. May as well get that PR approved and merged, actually. With that change and this, can do a new minor release instead of just the patch. It'll help qcng versions catch up to qcel, anyways.

dotsdl commented 2 years ago

Thanks @loriab! I'll review and merge #338, then pull that in to this PR today. Definitely appreciate getting a minor release out; will be a huge help for us in prod use!

WardLT commented 2 years ago

I just realized that I had the power to make that change, so I did so.

I'll merge after we merge #338, just to be careful about merging with passing tests and such

dotsdl commented 2 years ago

Thank you @WardLT! #338 is now merged, and I've merged master here to get those changes. Waiting on the tests now.