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

Fix dry_run in BaseSubmissionController. #19

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

t-reents
Copy link
Collaborator

@mbercx Just encountered a small bug when setting dry_run to True.

The submit_new_batch method fails when setting dry_run = True, as it attempts to index a set which raises a TypeError. This issue is fixed by converting the set to a list.

Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Thanks @t-reents! Indeed, in b910171 I removed some logic that converted the extras_to _submit set into a list. 🤦

Probably a good idea to write some tests at some point. 😅

For this PR: Maybe we can just cast the set assigned to the extras_to_submit variable to a list directly on line 185 using list()? I don't think any of the logic afterwards relies on this being a set.

@t-reents
Copy link
Collaborator Author

t-reents commented Feb 19, 2024

For this PR: Maybe we can just cast the set assigned to the extras_to_submit variable to a list directly on line 185 using list()? I don't think any of the logic afterwards relies on this being a set.

Yes, I was thinking about the same but didn't check it further. Indeed, we only iterate it afterwards, so I'll cast it directly into a list.

Probably a good idea to write some tests at some point. 😅

Haha, I was actually thinking the same when I encountered this one, and wanted to touch base with you on this in the office 😄 Happy to help, but I might need some support when setting up the mocking data.

The `submit_new_batch` method fails when setting `dry_run = True`, as it
attempts to index a `set` which raises a `TypeError`. This issue is fixed
by converting the `set` to a `list`.
@mbercx
Copy link
Member

mbercx commented Feb 19, 2024

Haha, I was actually thinking the same when I encountered this one, and wanted to touch base with you on this in the office 😄 Happy to help, but I might need some support when setting up the mocking data.

I should be back at the office tomorrow, happy to chat. 😄

@mbercx mbercx self-requested a review February 19, 2024 17:16
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Thanks again @t-reents!

@mbercx mbercx merged commit f83a7f9 into aiidateam:main Feb 19, 2024
1 check passed
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