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

shepherd - shepherd workflow manager #3843

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

saifulislampi
Copy link
Contributor

@saifulislampi saifulislampi commented May 21, 2024

Naming Update: "As of now, 'shepherd' has been provisionally selected as the name for this tool." - May 30, 2024


servicexd - Service Execution Daemon [old name]

This is a very early prototype for servicexd - the service execution daemon. The name is currently a placeholder. Please note that I am actively pushing changes to this project, so it may be unstable or broken at times.

@dthain
Copy link
Member

dthain commented May 21, 2024

Yep, you are on the right track here!

@dthain
Copy link
Member

dthain commented May 30, 2024

FYI, builds are failing though not your fault -- please rebase in order to pick up some recent fixes to the test infrastructure.

@dthain
Copy link
Member

dthain commented May 30, 2024

Based on our conversation so far, I am liking Shepherd or some variation on that for the name of the tool.
There are some systems out there using that name, but it's a pretty broad generic term.
I suggest that you use "shepherd" for the short name of the Unix command, directory structure, etc.
But then call it something more elaborate like "Shepherd Workflow Manager" as the full name for the webpage, documentation ,etc.

@dthain
Copy link
Member

dthain commented May 30, 2024

You have the right directory structure for integrating with the rest of the cctools.

One addition: In the test directory, please add a script TR_shepherd.sh with functions like prepare() run() and clean() that will execute the self-contained test example. See dttools/test/TR_jx.sh as an example of how to do it. (Obviously, we can't easily run the SADE example on every integration test, but's it nice to have that complete example there.)

@saifulislampi saifulislampi changed the title servicexd - Service Execution Daemon shepherd - shepherd worklow manager May 30, 2024
@saifulislampi
Copy link
Contributor Author

saifulislampi commented May 31, 2024

I will add the SADE workflow code here. But you are right, it would be difficult to run SADE workflow outside the whole prepared container. Let's brainstorm some other test cases together that we can run on every integration test. Right now, I am using some bash scripts that echo specific lines and transition into different states.

@dthain dthain changed the title shepherd - shepherd worklow manager shepherd - shepherd workflow manager Jun 7, 2024
subgraph cluster_service1 {
node [color=lightblue style=filled]
color=lightgrey style=filled
// service1_initialized [label="initialized"]
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove entries rather than just comment them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I agree. I will remove all comented code.

- graphviz
- matplotlib
- seaborn
# - pip:
Copy link
Member

Choose a reason for hiding this comment

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

Remove, not comment.

import sys


def extract_imports(file_content, source_dir):
Copy link
Member

Choose a reason for hiding this comment

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

You may want to use https://docs.python.org/3/library/ast.html rather than grep to get the imports, as regexps are not very robust. For example, I don't think the following case is handled correctly:

if something:
   import ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this as a temporary combiner for this project. I will remove this file entirely.

@@ -0,0 +1,371 @@
# Shepherd - Service Orchestration and Monitoring Tool
Copy link
Member

Choose a reason for hiding this comment

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

We usually use capital letters, like README.md.


preprocess_config(config, filepath)

print(f"DEBUG: Loaded and preprocessed config from {filepath}")
Copy link
Member

Choose a reason for hiding this comment

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

Since this is an independent module, you may want to use the standard python logging module.

for service_name, details in services.items():
# Auto-fill log and error files if not specified
if 'stdout_path' not in details:
details['stdout_path'] = f"{service_name}_stdout.log"
Copy link
Member

Choose a reason for hiding this comment

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

print("DEBUG: Validating and sorting programs")
required_keys = ['services']

for key in required_keys:
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why are you checking here and not letting the exception be raised in whatever code uses it?
Also, if you are going to raise it, it is a KeyError.

import time
import yaml

# --- config_loader.py ---
Copy link
Member

Choose a reason for hiding this comment

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

should these type of comments be removed?

@@ -0,0 +1,438 @@
from multiprocessing import Process
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want to commit this file? It seems to be a concatenation of all the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Thanks for pointing it out. I will remove all unncessary files and code update the PR soon.

@dthain
Copy link
Member

dthain commented Aug 7, 2024

@saifulislampi is it ok to close this PR in favor of the new Shepherd repository?

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.

None yet

3 participants