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

Changes to parallel workflow integration tests #40

Merged
merged 50 commits into from
Jul 28, 2022
Merged

Conversation

@kabilar kabilar added this to the 2022Q3 milestone Jul 12, 2022
Copy link
Contributor

@tdincer tdincer left a comment

Choose a reason for hiding this comment

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

Thank you, @CBroz1! Looks good in overall. Left some small comments.

@CBroz1 CBroz1 mentioned this pull request Jul 21, 2022
@CBroz1
Copy link
Contributor Author

CBroz1 commented Jul 21, 2022

Hi @tdincer - Merged your PR and fetched from upstream. Do you have time to finish review here today, so we can deploy to codebook tomorrow?

The only remaining item on this is the root_dir / processed_dir issue we discussed verbally. Lmk if you have additional thoughts.

To summarize:

  • The previous strategy:
    1. Output function processed returned the first available root if not otherwise specified by the user.
    2. Resulted in errors for projects not in the first available root.
  • My current strategy:
    1. Returns None when the output function isn't specified.
    2. When None, chooses the root that aligns with the current project - essentially removing the hardcoding of 'first'

This would be backwards incompatible in the unlikely situation in which someone (a) had multiple roots, with their main project dir not in the first, and (b) expected results to be stored in the first without defining processed function. In this case, the easy solve is to define the output function to point to the first.

@kabilar - Let me know if you have any thoughts on this

@tdincer
Copy link
Contributor

tdincer commented Jul 28, 2022

Thank you @CBroz1! I'm merging this now but I'll run some tests to check these changes next week.

@tdincer tdincer merged commit 8a63b19 into datajoint:main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants