Skip to content

Try/ddmd rose#15

Open
mgoliyad wants to merge 8 commits intomasterfrom
try/ddmd_rose
Open

Try/ddmd rose#15
mgoliyad wants to merge 8 commits intomasterfrom
try/ddmd_rose

Conversation

@mgoliyad
Copy link
Copy Markdown

@mgoliyad mgoliyad commented Sep 5, 2025

Please review new version of DDMD based on ROSE framework.
I have prepared dummy use case in
examples/dummy_pipeline

To run it:

python run_dummy_pipeline.py

or

sbatch anvil_CPU_sbatch.sh

@AymenFJA
Copy link
Copy Markdown

AymenFJA commented Sep 12, 2025

Hey @mgoliyad, thanks for the hard work. A few things to refine this PR which are unrelated to the core DDMD or ROSE work:

1- Create .github/workflows/tests.yml to enable the testing being picked by GitHub workflows
2- tests must start with the word test_, for example:

            tests\

                      unit\test_ddmd_compoenent_x.py

                      integration\test_ddmd_worfklow_with_x.py

Each test function must start with test_ to be picked by the pytest

I also left a few comments about the deployment work of the repo. Glad to explain more/help if needed.

Copy link
Copy Markdown

@AymenFJA AymenFJA left a comment

Choose a reason for hiding this comment

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

pre-review iteration comments

pyproject.toml Outdated
readme = "README.md"
requires-python = ">=3.9"

dependencies = ["rose", "radical.asyncflow", "radical.pilot"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You only need to add rose, and it should be from the GitHub link ATM, as we do not have a release.

By default, Rose will install asyncflow and radical.pilot

"pytest-cov"
]

doc = [
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see docs, but no documentation was provided. Is it going to be in the next dev. cycle?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, I want to have good use case and then decide what to put in docs

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@mgoliyad Documentation is essential for enabling users (even if you do not have active users for now) to understand how to interact with the API and other parts of the tool. It's important to build it proactively, rather than waiting for a specific use case to emerge. Thanks :)

pass


# @pytest.fixture
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if this code is not used, you can remove it.

@AymenFJA
Copy link
Copy Markdown

I will continue the review today.

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.

2 participants