Adding GALAXY_MEMORY_MB_PER_SLOT to k8s runner #11227
Conversation
| if 'cpu' in limits: | ||
| envs.append({'name': 'GALAXY_SLOTS', 'value': str(int(math.ceil(float(limits['cpu']))))}) | ||
| cpu_val = int(math.floor(float(limits['cpu']))) | ||
| cpu_val = cpu_val or 1 |
There was a problem hiding this comment.
This used to be ceiling for both limits and requests, but figured we should ceil for requests but floor for limits so that there is never more being requested than the hard limit set. I don't think that we'll ever need to set a limit 0<x<1 but just in case, since requesting 0 doesn't make sense, if the limit is set to be under 1, it will still ceil it to 1.
| mem_val = ByteSize(requests['memory']).to_unit('M', as_string=False) | ||
| envs.append({'name': 'GALAXY_MEMORY_MB', 'value': str(mem_val)}) | ||
| if cpu_val: | ||
| envs.append({'name': 'GALAXY_MEMORY_MB_PER_SLOT', 'value': str(math.floor(mem_val / cpu_val))}) | ||
| elif 'limits' in resources: |
There was a problem hiding this comment.
That's a pre-existing issue, but are requests and limits completely exclusive ? I assume it should be possible to set a CPU limit and a memory request or vice-versa ?
There was a problem hiding this comment.
The requests and limits are used for setting the actual resources of the pod in k8s, so the pod will "reserve" its request and get evicted if it goes over its limit. In general, we set both, but for the envs specified for the tool internally I think using just requests when they exist seems sensible to me
There was a problem hiding this comment.
So it's unlikely someone would mix requests and limits and the elif here is correct ?
There was a problem hiding this comment.
I'm not sure tbh. I mean I think in theory requests being used when both are available is the way to go, and using limits if no requests are set is sensible. There is the more niche example of what if someone sets only cpu request and memory limit, and I guess in that scenario (assuming it is possible and we want the scenario to stay possible, but we could also potentially just limit it to be more restrictive and just not allow that?) what I would think to be ideal would be to use those individually rather than take only the CPU request and completely ignore the memory limit which is what would happen now, but I think that's a design decision that I do not have enough background to make myself I guess. I would be happy to implement something like that where request would take precedence but a combination would be possible, and it seems super easy like just a few minutes to change it, I just don't know if it's desirable or not... For our purposes, we always set all 4 for the resource sets we include by default in the chart iirc, so it would not affect us either way.
Some tools eg: https://github.com/galaxyproject/tools-iuc/blob/master/tools/rgrnastar/rg_rnaStarSolo.xml#L113 ignore
GALAXY_MEMORY_MBand requireGALAXY_MEMORY_MB_PER_SLOT. This is an attempt at adding both envs to each kubernetes job, rather than only the former.