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

Initial workflow defs #163

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Initial workflow defs #163

wants to merge 4 commits into from

Conversation

ladinesa
Copy link
Collaborator

No description provided.

@ladinesa ladinesa requested a review from JFRudzinski January 28, 2025 09:04
@coveralls
Copy link

coveralls commented Jan 28, 2025

Pull Request Test Coverage Report for Build 13041776285

Details

  • 15 of 167 (8.98%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-4.4%) to 75.755%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nomad_simulations/schema_packages/workflow/init.py 0 4 0.0%
src/nomad_simulations/schema_packages/workflow/gw.py 0 22 0.0%
src/nomad_simulations/schema_packages/workflow/single_point.py 0 28 0.0%
src/nomad_simulations/schema_packages/workflow/geometry_optimization.py 0 41 0.0%
src/nomad_simulations/schema_packages/workflow/general.py 0 57 0.0%
Totals Coverage Status
Change from base Build 12272374018: -4.4%
Covered Lines: 2081
Relevant Lines: 2747

💛 - Coveralls


class Outputs(ArchiveSection):

class Outputs(Time):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does Outputs need to inherit from Time? Is there some situation where the simulation package is outputting such detailed timing info?

I guess maybe you are thinking of the workflows. Is the plan to reuse the output class directly for workflows?

Copy link
Collaborator Author

@ladinesa ladinesa Jan 28, 2025

Choose a reason for hiding this comment

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

The workflow tasks are built from the outputs and the time info are needed for determining the order of the tasks. There are a several codes that output the time for each step of a geometry optimization for example.

Copy link
Collaborator Author

@ladinesa ladinesa Jan 28, 2025

Choose a reason for hiding this comment

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

Indeed the task link at least for the general simulation workflow class references the output. For geometry optimization, I do a renormalization to extract the system to as I feel it is the natural input and output. Simulation.outputs as I understand, will not be extended to cover workflow methods and results, right? So I still would like to put the workflow-related quantities under workflow.method and workflow.results as in the old def.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, ok. I mean I am not against this I am just trying to understand.

The workflow tasks are built from the outputs
I guess you are talking about the standard simulation workflows here? And for a custom workflow, the user is specifying the ordering, so you don't need this, is that correct?

I do a renormalization to extract the system to as I feel it is the natural input and output
We should discuss this more generally, e.g., in the context of the workflow visualizer

Simulation.outputs as I understand, will not be extended to cover workflow methods and results, right?
I don't think we have explicitly discussed this. There should definitely be a separate outputs section under workflow (I thought of changing the name to outputs for consistency, but we should discuss). I don't think we made an explicit decision about this. From my side, I thought at some point it might make sense to re-use the outputs section definition under workflow, but this was more because of the flexibility of the Physical Property class, so probably that no longer makes sense.

I think I will put this on the agenda for tomorrow's meeting, and then we can resolve clearly this discussion? (You can probably continue as you think fit until then, it is unlikely that we override things that you deem necessary)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, ok. I mean I am not against this I am just trying to understand.

The workflow tasks are built from the outputs
I guess you are talking about the standard simulation workflows here? And for a custom workflow, the user is specifying the ordering, so you don't need this, is that correct?

I do a renormalization to extract the system to as I feel it is the natural input and output
We should discuss this more generally, e.g., in the context of the workflow visualizer

Simulation.outputs as I understand, will not be extended to cover workflow methods and results, right?
I don't think we have explicitly discussed this. There should definitely be a separate outputs section under workflow (I thought of changing the name to outputs for consistency, but we should discuss). I don't think we made an explicit decision about this. From my side, I thought at some point it might make sense to re-use the outputs section definition under workflow, but this was more because of the flexibility of the Physical Property class, so probably that no longer makes sense.

I think I will put this on the agenda for tomorrow's meeting, and then we can resolve clearly this discussion? (You can probably continue as you think fit until then, it is unlikely that we override things that you deem necessary)

This is for the automated workflows. Sure, we can discuss them in our meeting.

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