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

Glossary Corrections #3581

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Glossary Corrections #3581

wants to merge 24 commits into from

Conversation

Kanegraffiti
Copy link

Removed numbering from the terms
Did Alphabetical order for terms
Made sure each term had a label that could be referenced
Edited two index files to include the glossary

This is related to issue #3426
Recreates changes from this closed PR with corrections from @sophie-bui

Please review here @WardLT

@WardLT
Copy link
Contributor

WardLT commented Aug 13, 2024

A quick side note: no need to create a new PR for each set up changes. It's no problem that you have, but don't feel like you need to.

Copy link
Contributor

@WardLT WardLT left a comment

Choose a reason for hiding this comment

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

Only a minor comment about the "Launcher." Everything else looks great 💯

docs/userguide/glossary.rst Outdated Show resolved Hide resolved
Revised definition of Launcher based on feedback
@WardLT
Copy link
Contributor

WardLT commented Aug 13, 2024

That change looks good, but it seems like there's a problem with duplicate labels. The run is failing because

Warning, treated as error:
/home/runner/work/parsl/parsl/docs/historical/changelog.rst:947:more than one target found for 'any' cross-reference 'DataFuture': could be :std:ref:`DataFuture:` or :py:class:`parsl.app.futures.DataFuture`

Does that error make sense to use? It sounds like we might need to pick a different name for the reference label for DataFuture in your glossary

Changed the reference label for the DataFuture entry
@Kanegraffiti
Copy link
Author

That change looks good, but it seems like there's a problem with duplicate labels. The run is failing because

Warning, treated as error:
/home/runner/work/parsl/parsl/docs/historical/changelog.rst:947:more than one target found for 'any' cross-reference 'DataFuture': could be :std:ref:`DataFuture:` or :py:class:`parsl.app.futures.DataFuture`

Does that error make sense to use? It sounds like we might need to pick a different name for the reference label for DataFuture in your glossary

I have changed the DataFuture label, I considered changing others to match but I am worried about introducing new errors

@benclifford
Copy link
Collaborator

I am worried about introducing new errors

if you get new errors, then you get new errors and then you fix them in another commit - that isn't something to be scared of. my PRs fail CI about 50% of the time. what matters is not having errors when this PR is merged to the master branch.

@Kanegraffiti
Copy link
Author

I am worried about introducing new errors

if you get new errors, then you get new errors and then you fix them in another commit - that isn't something to be scared of. my PRs fail CI about 50% of the time. what matters is not having errors when this PR is merged to the master branch.

Thank you. I'll change the other labels now so they match

Changed the labels for the other entries for uniformity
@sophie-bui
Copy link
Collaborator

sophie-bui commented Aug 14, 2024

@benclifford – do we need to update Kelechi's permissions in our repo so she can contribute to our docs? I saw your message in our hacker's channel, so I want to double-check.

UPDATE: no permissions update is required. An approved reviewer will need to update and merge it.

@benclifford
Copy link
Collaborator

Here's an error that is happening in the documentation build now: (click on the red x of parsl / mail test-suite 3.8 and scroll all the way to the bottom)

Warning, treated as error:
/home/runner/work/parsl/parsl/docs/userguide/glossary.rst:99:'any' reference target not found: .result()

That sounds like something at line 99 of glossary.rst being treated as a reference, around the text .result() that you maybe don't want treated as a reference.

.result() method in the Future definition has been changed to double backticks (``.result()``), which should render it as inline code
@Kanegraffiti
Copy link
Author

Here's an error that is happening in the documentation build now: (click on the red x of parsl / mail test-suite 3.8 and scroll all the way to the bottom)

Warning, treated as error:
/home/runner/work/parsl/parsl/docs/userguide/glossary.rst:99:'any' reference target not found: .result()

That sounds like something at line 99 of glossary.rst being treated as a reference, around the text .result() that you maybe don't want treated as a reference.

The .result() method in the Future definition has been changed to double backticks, which should render it as inline code instead

@Kanegraffiti
Copy link
Author

We did it! Thank you for helping @sophie-bui @WardLT @benclifford

@Kanegraffiti
Copy link
Author

Thank you for your detailed review, I really appreciate your help, time and effort. I am applying your corrections right now! I hope to get more feedback from you afterwards @danielskatz

@danielskatz
Copy link
Member

Thank you for your detailed review, I really appreciate your help, time and effort. I am applying your corrections right now! I hope to get more feedback from you afterwards @danielskatz

Please ping me when you want me to review again.

Revised glossary definitions to improve clarity and accuracy based on feedback. Updated terms to use more precise language, particularly around the concepts of nodes, resources, and parallelism. Clarified the relationship between tasks, apps, and workflows.
@Kanegraffiti
Copy link
Author

Thank you for your detailed review, I really appreciate your help, time and effort. I am applying your corrections right now! I hope to get more feedback from you afterwards @danielskatz

Please ping me when you want me to review again.

Please can you look at this again, I have made corrections and it's ready for your review

@danielskatz
Copy link
Member

I've provided a few more comments. Also, I know @benclifford looked at this in terms of technically merging it, but it would be useful for him or @yadudoc or @kylechard to also review it for content.

Applied more feedback for three definitions and took out a fourth
@Kanegraffiti
Copy link
Author

I've provided a few more comments. Also, I know @benclifford looked at this in terms of technically merging it, but it would be useful for him or @yadudoc or @kylechard to also review it for content.

I have implemented the latest feedback, thank you. When differentiating between Parallelism and Concurrency I used an analogy with a similar theme in both definitions and compared that in Parallelism definition. Does it look okay?

Fixed a typo
@danielskatz
Copy link
Member

Yes, the concurrency vs parallelism does look ok to me now.

@danielskatz
Copy link
Member

I still would like one more review of the content, however

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.

5 participants