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.

Sign up for GitHub

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

Jump to bottom

Fix htcondor site adapter #173

Merged
merged 5 commits into from
Mar 23, 2021
Merged

Fix htcondor site adapter #173

merged 5 commits into from
Mar 23, 2021

Conversation

giffels
Copy link
Member

@giffels giffels commented Mar 23, 2021 •
edited
Loading

The HTCondor Site Adapter takes a wrong machine_meta_data_translation_mapping into account in some circumstances. Hence, we have to differentiate the translation mapping of the used OBS and the one used internal by the site adapter.
This bug has no effect if HTCondor is used as OBS, just if the OBS uses different conversion factors for Memory and Disk units this bug occurs.

This pull request fixes #170.

@giffels giffels requested review from a team, maxfischer2781 and HerrHorizontal and removed request for a team March 23, 2021 11:41
@giffels giffels self-assigned this Mar 23, 2021
@giffels giffels added the bug Something isn't working label Mar 23, 2021
@giffels giffels added this to the 0.6.0 - Release milestone Mar 23, 2021
@eileen-kuehn
Copy link
Member

Please also have a look at LAPIS where we already have a matrix of conversion mappings (for job io and pool io), just to ensure, that the right conversions are taken into consideration. In HTCondor there is some confusion with MB and MiB. Further, we should define what is being used internally and state it explicitly (just telling out of experience :D). Please ignore, if you already considered all of this 😇

@giffels
Copy link
Member Author

giffels commented Mar 23, 2021 •
edited
Loading

Please also have a look at LAPIS where we already have a matrix of conversion mappings (for job io and pool io), just to ensure, that the right conversions are taken into consideration. In HTCondor there is some confusion with MB and MiB. Further, we should define what is being used internally and state it explicitly (just telling out of experience :D). Please ignore, if you already considered all of this 😇

Thanks @eileen-kuehn. The memory in HTCondor is in MiB and disk space in KiB. That is already taken into account. I have opened an issue to update the documentation about what is used by TARDIS internally. #174

@giffels giffels closed this Mar 23, 2021
@giffels giffels reopened this Mar 23, 2021
maxfischer2781
maxfischer2781 previously approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

I suggest a rewording of the change fragment, but that is not a blocker.

docs/source/changes/173.fix_meta_data_translation_htcondor.yaml Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #173 (ec00641) into master (346fa3a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #173   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          43       43           
  Lines        1763     1769    +6     
=======================================
+ Hits         1755     1761    +6     
  Misses          8        8
Impacted Files Coverage Δ
tardis/adapters/sites/slurm.py 100.00% <ø> (ø)
tardis/resources/drone.py 100.00% <ø> (ø)
tardis/adapters/sites/htcondor.py 100.00% <100.00%> (ø)
tardis/interfaces/siteadapter.py 100.00% <100.00%> (ø)
tardis/utilities/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 346fa3a...ec00641. Read the comment docs.

maxfischer2781
maxfischer2781 previously approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@maxfischer2781 maxfischer2781 left a comment

Choose a reason for hiding this comment

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

Make it so! 👨‍🦲 🌟

@giffels giffels requested a review from maxfischer2781 March 23, 2021 15:58
HerrHorizontal
HerrHorizontal reviewed Mar 23, 2021
View reviewed changes
docs/source/changelog.rst Outdated
@@ -18,6 +18,7 @@ Fixed
-----

* Fixes a bug that the drone_minimum_lifetime parameter is not working as described in the documentation
* Fixes a bug in the HTCondor Site Adapter that could leading to wrong requirements when using non HTCondor OBS
Copy link

Choose a reason for hiding this comment

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

Here it should be "could lead"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I corrected it.

HerrHorizontal
HerrHorizontal previously approved these changes Mar 23, 2021
View reviewed changes
Copy link

@HerrHorizontal HerrHorizontal left a comment

Choose a reason for hiding this comment

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

Looks fine for me, except for some grammar (see inline comments).

@giffels @maxfischer2781
Fix typo in changelog
7793f57 
Co-authored-by: Max Fischer <maxfischer2781@gmail.com>
@giffels giffels dismissed stale reviews from HerrHorizontal and maxfischer2781 via 7793f57 March 23, 2021 16:20
@giffels giffels force-pushed the fix-htcondor-site-adapter branch from ec00641 to 7793f57 Compare March 23, 2021 16:20
@giffels giffels requested a review from HerrHorizontal March 23, 2021 16:21
HerrHorizontal
HerrHorizontal approved these changes Mar 23, 2021
View reviewed changes
Copy link

@HerrHorizontal HerrHorizontal left a comment

Choose a reason for hiding this comment

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

Everything fine now!

@giffels giffels merged commit d989653 into MatterMiners:master Mar 23, 2021
@giffels giffels deleted the fix-htcondor-site-adapter branch March 23, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HerrHorizontal HerrHorizontal HerrHorizontal approved these changes

@maxfischer2781 maxfischer2781 maxfischer2781 approved these changes

Assignees

@giffels giffels

Labels
bug Something isn't working
Projects
None yet
Milestone
0.6.0 - Release
Development

Successfully merging this pull request may close these issues.

HTCondor Site Adapter takes in some circumstances wrong a machine_meta_data_translation_mapping.
5 participants
@giffels @eileen-kuehn @codecov-io @maxfischer2781 @HerrHorizontal