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

Overhaul documentation on structuring software #3731

Merged
merged 4 commits into from
Dec 30, 2024

Conversation

WardLT
Copy link
Contributor

@WardLT WardLT commented Dec 29, 2024

Description

Makes changes that we discussed during a community call back in May:

  • Emphasize keeping decorators out of the main library
  • Discuss benefits of modules for serializing functions
  • Propose defining separate "workflow-ready" functions in module
  • General clean up of dicsussion

cc: @Andrew-S-Rosen

Changed Behaviour

N/A

Fixes

None

Type of change

  • Update to human readable text: Documentation/error messages/comments

Makes changes that we discussed during a community call back in May:

- Emphasize keeping decorators out of the main library
- Discuss benefits of modules for serializing functions
- Propose defining separate "workflow-ready" functions in module
- General clean up of dicsussion
@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented Dec 29, 2024

@WardLT: This is great! Thank you for putting this together. I think this is a big improvement.

[One practical challenge with this approach that I'll note from experience is that if the code maintainer is running tests without Parsl at all, they may run into problems when using Parsl if there are complex workflow patterns (think @join_app, for instance). In other words, things might run fine without Parsl but crash with Parsl. This is especially likely if you have contributors who are pushing "pure Python" code and know nothing about Parsl so aren't thinking about designing the codebase with Parsl in mind. I usually recommend having a separate Parsl-enabled test suite for this purpose, but I think this is a bit much for the docs.]

@WardLT
Copy link
Contributor Author

WardLT commented Dec 30, 2024

Thanks, Andrew! I'll see if I can work in a short mention of tests for join apps and such

WardLT and others added 2 commits December 30, 2024 08:18
E.g., join apps and other complex patterns.

Thanks, @Andrew-S-Rosen, for noting where he uses Parsl inside the
library.
@benclifford
Copy link
Collaborator

@WardLT want to merge that doc change now and then submit another one with more work, or work on it more before merging?

@WardLT
Copy link
Contributor Author

WardLT commented Dec 30, 2024

I'm happy with what I have here and am fine with merging it in. I might touch this part later, but plan to work on some other parts of the docs next

Co-authored-by: Ben Clifford <benc@hawaga.org.uk>
@benclifford benclifford added this pull request to the merge queue Dec 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 30, 2024
@benclifford benclifford added this pull request to the merge queue Dec 30, 2024
Merged via the queue into Parsl:master with commit a514a68 Dec 30, 2024
7 checks 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.

3 participants