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

Fix colocated db preparation bug when using JsrunSettings #339

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Aug 10, 2023

A model fails to properly configure colocated db settings when the run settings are of type JsrunSettings. This bug was introduced when protected members were made public (due to linter noting public use) but the change was partially rolled back.

The model method _set_colocated_db_settings attempts the following:

        if hasattr(self.run_settings, "prep_colocated_db"):
            self.run_settings.prep_colocated_db(common_options["cpus"])

There are no run settings classes with an attribute of that name. JsrunSettings does have _prep_colocated_db (note protected/underscore).

 def _prep_colocated_db(self, db_cpus: int) -> None:
        cpus_per_flag_set = False
        for cpu_per_rs_flag in ["cpu_per_rs", "c"]:
            if run_arg_value := self.run_args.get(cpu_per_rs_flag, 0):
                cpus_per_flag_set = True
                ...

This change fixes the incorrect call to a non-existent method by invoking self.run_settings._prep_colocated_db(...) instead.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #339 (9dc1f28) into develop (6125f3c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #339      +/-   ##
===========================================
+ Coverage    87.17%   87.19%   +0.02%     
===========================================
  Files           59       59              
  Lines         3531     3531              
===========================================
+ Hits          3078     3079       +1     
+ Misses         453      452       -1     
Files Changed Coverage Δ
smartsim/entity/model.py 95.77% <100.00%> (+0.70%) ⬆️

@ankona ankona requested review from MattToast and ashao August 10, 2023 17:34
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! LGTM, pending one change to the changelog

doc/changelog.rst Outdated Show resolved Hide resolved
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question, but otherwise my approval stands!

tests/test_colo_model_lsf.py Outdated Show resolved Hide resolved
@ankona ankona merged commit d80bbc0 into CrayLabs:develop Aug 15, 2023
10 checks passed
@MattToast MattToast added the bug: major A major bug label Sep 13, 2023
@ankona ankona deleted the oss507 branch April 4, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: major A major bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants