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: compare nodespecs based on ngpu/ncpu/mem first, before price #49

Merged
merged 11 commits into from
Mar 12, 2024

Conversation

mortenpi
Copy link
Member

@mortenpi mortenpi commented Mar 8, 2024

The documentation says that nodespec returns the smallest node, not necessarily the cheapest. This fixes the behavior where nodespec (and therefore submit_job) would pick the wrong node if there is a bigger, but cheaper one available, and clarifies the documentation.

It also makes the output for nodespecs() sorted, but that's an implementation detail, and hence not documented.

@mortenpi mortenpi requested a review from pfitzseb March 8, 2024 06:46
@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.61%. Comparing base (6799ba1) to head (1964bbf).

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #49       +/-   ##
===========================================
- Coverage   78.65%   40.61%   -38.05%     
===========================================
  Files          16       16               
  Lines        2244     2221       -23     
===========================================
- Hits         1765      902      -863     
- Misses        479     1319      +840     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pfitzseb
Copy link
Member

1.6 failure seems spurious?

@mortenpi
Copy link
Member Author

1.6 failure seems spurious?

Yep. I missed the doctest failures though.

@mortenpi mortenpi requested a review from pfitzseb March 12, 2024 23:16
@mortenpi mortenpi enabled auto-merge (squash) March 12, 2024 23:16
@mortenpi mortenpi merged commit 077bcdd into main Mar 12, 2024
11 checks passed
@mortenpi mortenpi deleted the mp/nodespec-order branch March 12, 2024 23:42
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.

3 participants