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

Remove granularity #209

Merged
merged 3 commits into from
Sep 24, 2021
Merged

Remove granularity #209

merged 3 commits into from
Sep 24, 2021

Conversation

mschnepf
Copy link
Member

With granularity new drones are requested when demand>supply+granularity. Remove granularity in Standardiser to enable the creation of new drones when demand>supply. I think that is more intuitive and solves problems as discussed in MatterMiners/cobald#93.

With `granularity` new drones are requested when `demand>supply+granularity`. Remove granularity in Standardiser to enable the creation of new drones when `demand>supply`.  I think that is more intuitive and solves problems as discussed in [Fix stepwise controller documentation](MatterMiners/cobald#93).
@giffels
Copy link
Member

giffels commented Sep 13, 2021

@mschnepf: Could you fix unittests and code style, please?

@giffels giffels requested review from giffels, a team and eileen-kuehn and removed request for a team September 13, 2021 13:35
@giffels
Copy link
Member

giffels commented Sep 13, 2021

How many drones are requested if:

demand = 16
supply = 8

if the drone granularity is 8?

With using granularity it is one.

@mschnepf
Copy link
Member Author

Oh. Yes, I will fix it.

In both cases, it requests only one additional drone. Without granularity it will request two additional drones if
demand = 17
supply = 8
With granularity>1 it would request only one additional drone.

@eileen-kuehn
Copy link
Member

I would like to understand the underlying problem, so stick with me for some questions 😁
First, there must have been a reason to introduce granularity. Can you shed some light on this please?
Further, what is the reason that you think it should be removed. Setting the demand based on current demand as discussed in the other ticket seems a bit different. So what is the underlying problem and where does it come from?

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021

Codecov Report

Merging #209 (9728248) into master (c55d57b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #209   +/-   ##
=======================================
  Coverage   99.55%   99.55%           
=======================================
  Files          42       42           
  Lines        1795     1795           
=======================================
  Hits         1787     1787           
  Misses          8        8           
Impacted Files Coverage Δ
tardis/resources/poolfactory.py 100.00% <ø> (ø)
tardis/utilities/attributedict.py 100.00% <ø> (ø)
tardis/adapters/sites/cloudstack.py 100.00% <100.00%> (ø)
tardis/adapters/sites/htcondor.py 100.00% <100.00%> (ø)
tardis/adapters/sites/moab.py 100.00% <100.00%> (ø)
tardis/adapters/sites/openstack.py 100.00% <100.00%> (ø)
tardis/adapters/sites/slurm.py 100.00% <100.00%> (ø)
tardis/resources/dronestates.py 99.32% <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 c55d57b...9728248. Read the comment docs.

@mschnepf
Copy link
Member Author

I think that the reason for the use of granularity was to quantify demand similar to supply.
Since the standardiser rounding down the demand, TARDIS requests more resources if the demand>supply+granularity. TARDIS artificially reduces the demand. For drones with 8 CPU cores, the demand of 15 CPU cores is not met because only 8 CPU cores are requested.

If the demand is changed based on the supply this can result in a limit of demand and supply.

@eileen-kuehn
Copy link
Member

I think that the reason for the use of granularity was to quantify demand similar to supply.
Since the standardiser rounding down the demand, TARDIS requests more resources if the demand>supply+granularity. TARDIS artificially reduces the demand. For drones with 8 CPU cores, the demand of 15 CPU cores is not met because only 8 CPU cores are requested.

If the demand is changed based on the supply this can result in a limit of demand and supply.

I must further try to ask, because I still don't get the real underlying problem.
Why do you artificially argument with a demand of 8 cores? Isn't the pool obliged to define that a supply of 1 is translated to whatever it is defined to be, e.g. 8 cores? So why do we have to put an additional granularity there? Isn't it sufficient to say that we need more of what the pool has to offer, that is another portion of 8 cores?
So why has this granularity been introduced. Which issue does it fix other than introducing new issues?

@mschnepf
Copy link
Member Author

The idea of setting demand and supply equal to CPU-cores is to weigh different kinds of machines. An eight CPU-core drone provides more slots than a four CPU-core drone. But you are right, it should also work fine without that weight.

Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Lets hope that it does solve the raised issues :)
Great, that you also adapted the error handling 👍

Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

@mschnepf thanks a lot for your contribution. I have no further comments. Looks good to me!

@giffels giffels merged commit 9d45bbc into MatterMiners:master Sep 24, 2021
giffels added a commit to giffels/tardis that referenced this pull request Apr 19, 2022
giffels added a commit to giffels/tardis that referenced this pull request Jan 20, 2023
giffels added a commit to giffels/tardis that referenced this pull request Feb 24, 2023
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.

4 participants