Phase weight rework 2 - #34
Closed
lauren-walters wants to merge 6 commits into
Closed
Conversation
Ray's num_cpus=1 default per refinement task let as many concurrent tasks run as detected CPUs, decoupled from the n_threads a task's BGMN subprocess actually used -- at the old default (n_threads=8, 12 concurrent tasks) this oversubscribed the machine ~8-10x in OS threads, which can starve Ray's own GCS/raylet under sustained load. - Add dara.hardware.detect_available_cores(): portable core-count detection (SLURM allocation, then OS affinity/cgroup quota, then os.cpu_count()). - Add DaraSettings.RAY_NUM_CPUS (default: detected cores) and DaraSettings.BGMN_N_THREADS (default: 1), both overridable via DARA_RAY_NUM_CPUS / DARA_BGMN_N_THREADS env vars or ~/.dara.yaml. - search_phases()'s ray.init() now passes num_cpus=RAY_NUM_CPUS explicitly instead of relying on Ray's own auto-detection (which ignores SLURM allocations and cgroup/container limits). - Each refinement Ray task now requests num_cpus=n_threads via .options(...) at submission, instead of a hardcoded @ray.remote(num_cpus=1), so Ray throttles concurrency to floor(cores / n_threads) automatically -- with defaults this is 12 workers x 1 thread on a 12-core machine; overriding n_threads upward safely drops concurrency to match. Threads/concurrency/config only -- no change to search logic, pruning, or node counts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The intensity-order pruning heuristic assumed phases are discovered in decreasing order of abundance, terminating any branch where a newly-added phase had more calculated peak intensity than one added earlier -- even when that branch's overall fit was clearly better, discarding the correct answer. - Add should_prune_low_weight_fraction(intensity_out_of_order, parent_rwp, child_rwp, material_improvement_threshold=LOW_WEIGHT_FRACTION_RWP_IMPROVEMENT), a small standalone helper in dara.search.tree. The intensity-order signal is still computed exactly as before; the helper only decides whether it's fatal. LOW_WEIGHT_FRACTION_RWP_IMPROVEMENT defaults to 0.10: an out-of-order branch is now pruned only if it does NOT improve Rwp by at least 10% relative to its parent. With no parent to compare against (or a degenerate parent_rwp <= 0), the ordering signal alone still decides, same as before. - Wire the helper into BaseSearchTree.expand_node() in place of the old inline boolean check. - Add tests/test_search_tree.py: 9 direct unit tests of the helper plus 3 integration-level tests driving the real expand_node() (score_phases/ refine_phases mocked to avoid needing BGMN; calculate_fom_and_strain patched since it only reads a phase's own CIF file) confirming a materially-better branch is retained, a marginally-better one is still pruned, and in-order additions are never flagged regardless of fit. Decision-only change: no recovery/branch-respawning mechanism, no change to how many nodes/branches get explored beyond retaining branches this rule would previously have discarded. On a deterministic 30-CIF subset of dara-clustering-problem-2 (first 30 alphabetically), before/after node count, statuses, and best Rwp are identical (15 nodes, best Rwp 29.73) -- the out-of-order case that arose there didn't clear the 10% bar either way, consistent with the unit/integration tests being the primary proof this works as designed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ruff format: collapse two multi-line assignments in tree.py's expand_node() and the whole of the new tests/test_search_tree.py to match the project's configured formatter (line-length=120). No other files in this branch's diff needed reformatting (hardware.py and settings.py were already compliant); pre-existing formatting drift in core.py/tree.py outside this branch's added lines was left untouched. - ruff check across the whole repo: zero new findings in this branch's files. The 13 existing findings are all pre-existing, in files this branch never touched (notebooks, scripts/filter_cod.py, scripts/filter_icsd.py, tests/test_cif2str.py) -- left as-is per scope. - Reviewed this branch's comments/docstrings against origin/main...HEAD for LLM-flavored padding, step narration, and restated-the-obvious comments. Found none to remove: every comment already documents non-obvious rationale (the hardware-detection priority order, the LOW_WEIGHT_FRACTION_RWP_IMPROVEMENT threshold's meaning, the .options(num_cpus=...) oversubscription-safety note) or is a concise, accurate docstring on a public helper. No comment/docstring text changed. No logic or behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
LOW_WEIGHT_FRACTION_RWP_IMPROVEMENT: 0.10 -> 0.08. Updated the constant's doc comment to match. tests/test_search_tree.py: the exactly-at-threshold and just-under-threshold tests already derived their boundary values from LOW_WEIGHT_FRACTION_RWP_IMPROVEMENT itself, so they needed no value changes to track the new threshold -- only the exactly-at-threshold case needed a tiny epsilon nudge, since 40.0 * (1 - 0.08) does not round-trip to exactly 0.08 in floating point and was landing on the wrong side of the `<` vs `<=` boundary (0.07999999999999989 < 0.08), an artifact of the boundary construction, not a bug in should_prune_low_weight_fraction. The marginal (5%, still below 8%) and material (50%) cases needed no changes; only a stale "10%" comment was updated to "8%". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merges upstream/main (15 commits ahead of where this branch forked, including PR CederGroupHub#30's non-reverted RefinementMetrics addition to RefinementResult) into this branch -- no conflicts. RefinementResult.refinement_metrics is now a required field (RefinementMetrics, whose only required field is rwp). The _make_refinement_result test helper in tests/test_search_tree.py built a RefinementResult without it, so pydantic validation failed: "refinement_metrics Field required [type=missing]" -- this is what broke CI (GitHub tests PR branches merged into the current base branch tip, not the branch in isolation, which is why this only showed up there and not in any isolated-branch reproduction). Fix: pass a real RefinementMetrics(rwp=rwp) instance, matching how production code constructs one (see get_result() / refine.py). No model changes, no extra="ignore", nothing stubbed out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
|
Closed upon request of @lauren-walters |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Major changes:
Todos
If this is work in progress, what else needs to be done?
Checklist
Tip: Install
pre-commithooks to auto-check types and linting before every commit: