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

APPS-1271 Update improve manifest docs #354

Merged
merged 27 commits into from
Jul 25, 2022
Merged

Conversation

Gvaihir
Copy link
Contributor

@Gvaihir Gvaihir commented Jul 14, 2022

(fix) ExpertOptions.md - clarification of manifest inputs. Fixed example

(fix) ExpertOptions.md - clarification of manifest inputs. Fixed example
@Gvaihir Gvaihir added documentation minor Minor change - does not require unit or integration testing labels Jul 14, 2022
(fix) ExpertOptions.md - review changes
@Gvaihir Gvaihir requested a review from commandlinegirl July 15, 2022 19:57
Copy link
Collaborator

@emiloslavsky emiloslavsky left a comment

Choose a reason for hiding this comment

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

some changes, a request for examples and some questions to improve my understanding of manifest mode.

doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
Gvaihir and others added 4 commits July 18, 2022 12:45
(fix) ExpertOptions.md - review changes
(fix) ExpertOptions.md - review changes
(fix) ExpertOptions.md - review changes
@Gvaihir Gvaihir requested a review from emiloslavsky July 18, 2022 20:32
doc/ExpertOptions.md Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
Gvaihir and others added 9 commits July 20, 2022 23:12
in GH web

Co-authored-by: emiloslavsky <[email protected]>
in GH web

Co-authored-by: emiloslavsky <[email protected]>
from GH web

Co-authored-by: emiloslavsky <[email protected]>
from GH web

Co-authored-by: emiloslavsky <[email protected]>
from GH web

Co-authored-by: emiloslavsky <[email protected]>
GH web

Co-authored-by: emiloslavsky <[email protected]>
GH web

Co-authored-by: emiloslavsky <[email protected]>
GH web

Co-authored-by: emiloslavsky <[email protected]>
GH web

Co-authored-by: emiloslavsky <[email protected]>
@Gvaihir Gvaihir requested a review from emiloslavsky July 21, 2022 06:25
@Gvaihir
Copy link
Contributor Author

Gvaihir commented Jul 21, 2022

Thanks for the reviews/fixes @emiloslavsky

doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
Gvaihir and others added 2 commits July 21, 2022 10:12
Co-authored-by: emiloslavsky <[email protected]>
Co-authored-by: emiloslavsky <[email protected]>
doc/ExpertOptions.md Outdated Show resolved Hide resolved
emiloslavsky and others added 2 commits July 21, 2022 15:50
@Gvaihir Gvaihir requested a review from emiloslavsky July 22, 2022 02:24
@Gvaihir
Copy link
Contributor Author

Gvaihir commented Jul 22, 2022

@emiloslavsky latest changes incorporated. thanks!

doc/ExpertOptions.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@emiloslavsky emiloslavsky left a comment

Choose a reason for hiding this comment

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

missing ` otherwise LGTM

Co-authored-by: emiloslavsky <[email protected]>
@Gvaihir Gvaihir requested a review from emiloslavsky July 22, 2022 16:11
emiloslavsky
emiloslavsky previously approved these changes Jul 22, 2022
(fix) ExpertOptions.md - review changes
@Gvaihir
Copy link
Contributor Author

Gvaihir commented Jul 22, 2022

@commandlinegirl - could you approve if my comments satisfy your request for changes? I tried to outdate them but it didn't work

doc/ExpertOptions.md Outdated Show resolved Hide resolved
Comment on lines +1444 to +1447
In extreme cases, running compiled workflows can fail due to DNAnexus platform limits on the total size of the input and
output JSON documents of a job. An example is a task with many inputs/outputs that is called in scatter over a large collection.
In such a case, you can enable manifest support at compile time with the `-useManifests` option.
This option causes each generated applet or workflow to accept inputs as an array of manifests, and to produce outputs as a single manifest.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In extreme cases, running compiled workflows can fail due to DNAnexus platform limits on the total size of the input and
output JSON documents of a job. An example is a task with many inputs/outputs that is called in scatter over a large collection.
In such a case, you can enable manifest support at compile time with the `-useManifests` option.
This option causes each generated applet or workflow to accept inputs as an array of manifests, and to produce outputs as a single manifest.
In extreme cases, running compiled workflows can fail due to DNAnexus platform limits on the total size of the input and output JSON documents of a job. An example is a task with many inputs/outputs that is called in scatter over a large collection. In such a case, you can enable manifest support at compile time with the `-useManifests` option. This option causes each generated applet or workflow to accept inputs as an array of manifests, and to produce outputs as a single manifest.

It's a minor thing but it's easier to read (now and in the future) if there are no newlines introduced.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below wherever the breakline is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this new line here looks ugly to show the difference in versions. In the raw source it looks fine and there's no change in the rendered variant. But without this break line - the source looks like one stretched line and you have to scroll sidewise to read it. So I would really insist on having those break lines in the markdown docs from now on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it looks now:

In extreme cases, running compiled workflows can fail due to DNAnexus platform limits on the total size of the input and

This is how the rest of the doc looks without new lines:

When calling a workflow with `dx run`, jobs and analyses launched by this workflow will have their temporary workspaces to store resources and intermediate outputs. By default, when a job or an analysis has transitioned to a terminal state (done, failed, or terminated), its temporary workspace will be destroyed by the system.

To me second example is harder to read compared to my version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please click the links to actually see it in the raw .md

doc/ExpertOptions.md Outdated Show resolved Hide resolved
doc/ExpertOptions.md Outdated Show resolved Hide resolved
Gvaihir and others added 6 commits July 22, 2022 16:29
(fix) ExpertOptions.md - review changes
(fix) ExpertOptions.md - review changes
(fix) ExpertOptions.md - review changes
(fix) ExpertOptions.md - review changes
(fix) ExpertOptions.md - review changes
Co-authored-by: Aleksandra Zalcman <[email protected]>
@Gvaihir Gvaihir requested a review from commandlinegirl July 25, 2022 16:41
section can be used to pass manifest output from a stage of one workflow (including the `output` stage) as input to another workflow. A
typical use case for this scenario is when a user wants to pass manifest output file from a stage (including `output` stage)
directly to a new workflow. Also, this scenario might be useful when debugging individual stages of a failing workflow.

## Manifest JSON

When manifest support is enabled, each applet has an `input_manifest___` input field of type `hash`, which means that it
When manifest support is enabled, applet/workflow outputs which are passed from one stage to another (or to the final output
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to inform the user of what the input_manifest___ is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not as simple. It does have this field, as well as a bunch of others, which I could not find the documentation for. APPS-1309 ticket should address that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Thanks!

@Gvaihir Gvaihir merged commit 2804179 into develop Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Minor change - does not require unit or integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants