Skip to content

Mem/disk mib fix and unit test#1

Merged
BEFH merged 2 commits intoBEFH:mem_disk_fixfrom
hermidalc:mem_disk_fix
Aug 22, 2024
Merged

Mem/disk mib fix and unit test#1
BEFH merged 2 commits intoBEFH:mem_disk_fixfrom
hermidalc:mem_disk_fix

Conversation

@hermidalc
Copy link
Copy Markdown

@hermidalc hermidalc commented Aug 22, 2024

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

@BEFH
Copy link
Copy Markdown
Owner

BEFH commented Aug 22, 2024

maybe we should also test the calculation of mem_mb from mem when mem is in GB/TB? I changed that code as well. Also, is there any way you can do the PR to the mem_disk_fix branch instead of the main branch? Otherwise, I do not think anything else is required.

For mem="1 GB", mem_mb should be 1000 now and mem_mib should be 954. This might work to test everything but I don't know if it makes sense to have separate tests for each thing.

@hermidalc
Copy link
Copy Markdown
Author

maybe we should also test the calculation of mem_mb from mem when mem is in GB/TB? I changed that code as well. Also, is there any way you can do the PR to the mem_disk_fix branch instead of the main branch? Otherwise, I do not think anything else is required.

For mem="1 GB", mem_mb should be 1000 now and mem_mib should be 954. This might work to test everything but I don't know if it makes sense to have separate tests for each thing.

Sorry if I'm not up-to-date on standards, but for disk I would expect I think 1 GB == 1000 but not for mem? I think that should be 1024 still?

@BEFH
Copy link
Copy Markdown
Owner

BEFH commented Aug 22, 2024

The issue is that the code is currently fixed but extremely literal. If you specify G or GB (technically an SI unit), it's GB. If you specify GiB, it's the binary unit. I don't know why you would ever would want to use SI units for memory, so what you're saying is potentially a good idea, but it also potentially breaks the distinction between mem_mb and mem_mib. A happy medium would be to assume binary in mem but not in disk when using humanfriendly.parse_size()

@hermidalc
Copy link
Copy Markdown
Author

The issue is that the code is currently fixed but extremely literal. If you specify G or GB (technically an SI unit), it's GB. If you specify GiB, it's the binary unit. I don't know why you would ever would want to use SI units for memory, so what you're saying is potentially a good idea, but it also potentially breaks the distinction between mem_mb and mem_mib. A happy medium would be to assume binary in mem but not in disk when using humanfriendly.parse_size()

Definitely should keep with the existing impl to not break people's code. I just tested it on 8.16 and 4 GB yields mem_mb=3815

@BEFH
Copy link
Copy Markdown
Owner

BEFH commented Aug 22, 2024

4 GB yielding mem_mb=3815 is incorrect behavior. It should be either 4000 or 4295 depending on implementation. The current value is a huge bug.

@hermidalc
Copy link
Copy Markdown
Author

hermidalc commented Aug 22, 2024

4 GB yielding mem_mb=3815 is incorrect behavior. It should be either 4000 or 4295 depending on implementation. The current value is a huge bug.

Yep I tested it with local and on the slurm cluster and 4G is 3815 MB in Snakemake 8.16, odd. I guess your fix will correct that too?

@BEFH
Copy link
Copy Markdown
Owner

BEFH commented Aug 22, 2024 via email

@hermidalc
Copy link
Copy Markdown
Author

I added gb and tb to mib tests

@BEFH
Copy link
Copy Markdown
Owner

BEFH commented Aug 22, 2024

Thanks! is there any way you could change the base branch for the pull request from main to mem_disk_fix?

@hermidalc
Copy link
Copy Markdown
Author

Thanks! is there any way you could change the base branch for the pull request from main to mem_disk_fix?

Damn I don't know why it did that I was working locally on the mem_disk_fix branch I just double checked it. Have to figure out how to change it

@BEFH BEFH changed the base branch from main to mem_disk_fix August 22, 2024 17:48
@hermidalc
Copy link
Copy Markdown
Author

Thank you @BEFH it didn't look like I had the permissions to do it because I didn't get a dropdown menu to change the base branch I can only see a link there

@BEFH BEFH merged commit 8122ab2 into BEFH:mem_disk_fix Aug 22, 2024
@BEFH
Copy link
Copy Markdown
Owner

BEFH commented Aug 22, 2024

I figured out how to change it myself. I will be adding the MB to the outputs as well.

@hermidalc hermidalc deleted the mem_disk_fix branch August 22, 2024 17:51
BEFH pushed a commit that referenced this pull request Feb 4, 2026
Pixi install github action is failing with "failed to parse pypi name
mapping" errors likely due to rate limiting when 30+ jobs are kicked off
nearly simultaneously

I tested this fix on snakemake#3820 since the tests kept failing due to the
`pixi` install action failing. After committing this change, [the
actions ran
successfully](https://github.com/snakemake/snakemake/actions/runs/20684893321).

In [this failing
run's](https://github.com/snakemake/snakemake/actions/runs/20682879051/job/59383597583#step:3:3261)
debug logs we see:
```
pixi install -e py311
[...]
   WARN resolve_conda{group=py313 platform=win-64}: reqwest_retry::middleware: Retry attempt #1. Sleeping 1.225245051s before the next attempt
  Error:   × failed to parse pypi name mapping
    ├─▶ error decoding response body
    ╰─▶ expected value at line 1 column 1
```
This warning is repeated many times until finally pixi stops retrying -
this is what suggested to me that some sort of rate limit was the issue.


One downside is that this does make the CI take a bit longer to run. We
could consider using the `cache` feature of the pixi action. And turning
up the max-parallel, or reducing the number of test-groups


### QC
<!-- Make sure that you can tick the boxes below. -->

* [ ] The PR contains a test case for the changes or the changes are
already covered by an existing test case.
* [ ] The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).



<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Chores**
* Updated development toolchain dependencies for improved build and test
infrastructure.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.

2 participants