Skip to content

BUG: sample_ids are strings for partitioning - #398

Merged
ebolyen merged 3 commits into
qiime2:devfrom
cherman2:no_mag_manifest
Jul 16, 2026
Merged

ebolyen merged 3 commits into
qiime2:devfrom
cherman2:no_mag_manifest

Conversation

@cherman2

@cherman2 cherman2 commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Description

Enforces that sample id's are treated like strings when filtering manifests during partitioning.

a forum post found that if the id's are all numeric, this line filters them out and then they get a rachis.core.exceptions.ValidationError: /tmp/qiime2/user/tmp/rachis-OutPath-ih9wwzbg/MANIFEST is not a(n) MultiMAGManifestFormat file: No sample records found in manifest, only observed comments, blank lines, and/or a header row.

https://forum.qiime2.org/t/evaluate-bins-with-busco-in-moshpit-tutorial-4-1-6-section/34073/16

I am using this PR as a "Issue placeholder". This code change fixes the issues but I am open to alternative solutions

AI Disclosure

  • NO AI USED.
  • AI USED.

AI Usage Details

@cherman2 cherman2 changed the title ids are always strings BUG: sample_ids are strings for partitioning Jun 30, 2026
manifest = pd.read_csv(mags.path / "MANIFEST", index_col=None)
manifest = manifest[manifest["sample-id"].isin(all_samples)]
manifest = \
manifest[manifest["sample-id"].astype("string").isin(all_samples)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might be able to coerce first (I think all of these columns are strings)

@misialq misialq removed their assignment Jul 16, 2026
@misialq

misialq commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Hey @ebolyen, this is ready for review (if needed) or merging. I added another set of MAGs with numerical IDs in the test and applied type coercion, as you suggested - it did fix the issue.

@ebolyen ebolyen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for the tests!

@ebolyen
ebolyen merged commit e76b408 into qiime2:dev Jul 16, 2026
6 checks passed
@misialq misialq self-assigned this Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants