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

vine: a manager argument to rename runtime directory #4041

Merged

Conversation

JinZhou5042
Copy link
Member

@JinZhou5042 JinZhou5042 commented Jan 27, 2025

Proposed Changes

Fix #3942

  • runtime path: the upper level directory that archives log directories from various runs, by default vine-run-info
  • runtime directory: the direct directory that keeps information of a particular workflow, it is one of the subdirectories of runtime path, by default %Y-%m-%dT%H%M%S

This provides the user with an easy way to rename their runtime directory by passing a run_info_dir argument to the manager.

Though the users can set VINE_RUNTIME_INFO_DIR to rename it, it might be confusing that they can specify the runtime path by setting an argument but need to rely on a different way os.environ["VINE_RUNTIME_INFO_DIR"] for renaming the runtime directory.

Merge Checklist

The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.

  • make test Run local tests prior to pushing.
  • make format Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)
  • make lint Run lint on source code prior to pushing.
  • Manual Update: Update the manual to reflect user-visible changes.
  • Type Labels: Select a github label for the type: bugfix, enhancement, etc.
  • Product Labels: Select a github label for the product: TaskVine, Makeflow, etc.
  • PR RTM: Mark your PR as ready to merge.

@JinZhou5042 JinZhou5042 self-assigned this Jan 27, 2025
@JinZhou5042
Copy link
Member Author

JinZhou5042 commented Jan 27, 2025

The problem I can foresee is that if the users forget to change the argument run_info_dir between different runs, their previous files might be appended with the new log text.

Do we want to add a time-formatted suffix if there has been a directory with the specified run_info_dir name? Or to emit a warning message to let them manually handle the conflict?

@JinZhou5042 JinZhou5042 requested review from btovar and dthain January 27, 2025 20:45
@JinZhou5042 JinZhou5042 changed the title vine: a manager method to rename runtime directory vine: a manager argument to rename runtime directory Jan 27, 2025
@btovar
Copy link
Member

btovar commented Jan 27, 2025

We don't want to change the name of this directory once the manager is defined, though. As you pointed out, it may cause trouble if renamed once a log started, or files have been staged. Note that the env var only works at the start, changing it does not change the directory once is created.

@JinZhou5042
Copy link
Member Author

Yeah, I agree with you, so we can do it in two ways

  • Set os.environ["VINE_RUNTIME_INFO_DIR"] = xxx in Python before creating the manager
  • Specify run_info_dir as one of the manager's arguments, as in this pr

@btovar Which one do you prefer? Is the conflict caused by this env var a problem that needs to be fixed?

@btovar
Copy link
Member

btovar commented Jan 28, 2025

That's what run_info_path is for, right?

@btovar
Copy link
Member

btovar commented Jan 28, 2025

I'm afraid that the distinction between the names/functions of these two is very small and it will cause confusion.

@JinZhou5042
Copy link
Member Author

JinZhou5042 commented Jan 28, 2025

My directory tree is like this:

vine-run-info
	├── 2025-01-27T163604
	│   ├── library-logs
	│   └── vine-logs
	│       ├── debug
	│       ├── performance
	│       ├── taskgraph
	│       ├── transactions
	│       └── workflow.json
	├── 2025-01-28T095429
	│   ├── library-logs
	│   └── vine-logs
	│       ├── debug
	│       ├── performance
	│       ├── taskgraph
	│       ├── transactions
	│       └── workflow.json
	├── 2025-01-28T095515
	    ├── library-logs
	    └── vine-logs
	        ├── debug
	        ├── performance
	        ├── taskgraph
	        ├── transactions
	        └── workflow.json

Where run_info_path represents vine-run-info and run_info_dir stands for 2025-01-27T163604, 2025-01-28T095429 and 2025-01-28T095515.

@dthain
Copy link
Member

dthain commented Jan 28, 2025

I would prefer to avoid the use of environment variables. In my experience they lead to user confusion and troubleshooting difficulties. Better to make options explicit as command-line arguments or arguments to functions, so that users can clearly see where things are going and why.

(Limited exceptions for things like TCP_PORT_RANGE that have a common effect across many programs.)

@btovar
Copy link
Member

btovar commented Jan 28, 2025

Agreed about env vars. Here we are using them so that we can set the debug log path before the manager is created so that we can get debug info of the creation. In python a user would not see the env var, the issue would be apparent in C. We could add a static var so that it works in C, but that's horrible in its own way.

@btovar
Copy link
Member

btovar commented Jan 28, 2025

@JinZhou5042 I see that the mess of names it's actually my fault and that you are just copying the names from C. We should look for a better name for runtime_dir so that it is clear is just for this manager, and that it is relative to the other path.

@JinZhou5042
Copy link
Member Author

Oh, now I'm understanding it deeper! @btovar Do you want me to proceed this PR with a better name, or address the underlying naming policies in a separate PR?

@btovar
Copy link
Member

btovar commented Jan 28, 2025

let's keep this pr and find better names.

@JinZhou5042
Copy link
Member Author

Sounds good!

@dthain
Copy link
Member

dthain commented Feb 3, 2025

As a general rule, I would prefer to keep the current C API which (mostly) separates object creation from configuration:

m = vine_create(port);
vine_set_password(m,password);
vine_set_scheduler(q,SCHEDULE_TO_FILES);

The idea here is that vine_create does very little beyond just creating the data structures, and then we have the freedom to add new features and capabilities without constantly modifying the manager constructor.

In this particular case, is there a compelling reason why we can't do this?

m = vine_create(port);
vine_set_runtime_info(m, path);

With that said, I have no objection if the Python constructor looks like this:

m = taskvine.Manager(port,password=p,scheduler=s,runtime_info=path)

Because that easily translates into the structure above.

@btovar
Copy link
Member

btovar commented Feb 3, 2025

The reason is that changing the runtime dir once the manager is up it is not really supported. We would need to check that no file or task refers to the old directory. I rather have it so that it is a parameter that can't be changed once set (like we do with the manager's port).

@dthain
Copy link
Member

dthain commented Feb 4, 2025

So then let's bite the bullet and make it a proper argument to the constructor.

(But we can't do that for everything!)

@JinZhou5042
Copy link
Member Author

Sounds like the right way to do it! How do we want to name the two parameters about the runtime path and the runtime directory?

@btovar
Copy link
Member

btovar commented Feb 4, 2025

run_info_path and run_info_template?

@btovar
Copy link
Member

btovar commented Feb 4, 2025

run_info_path remains the same. run_info_dir is the one that becomes run_info_template.

@JinZhou5042
Copy link
Member Author

The main changes are:

  • run_info_path specifies the directory where log entries across different runs are archived
  • run_info_template specifies the workflow related directory where debug logs and staging directory are written into
    • If run_info_template has alreay exists, append a suffix with the current time, by a %Y-%m-%dT%H%M%S format, This avoids logs from different runs from being written to the same file when users forget to update the argument for a new run
    • If run_info_template is not set, explicitly unset that environmental variable, to avoid potential issues caused by leftover env vars from the previous runs
  • set_runtime_info_pathis removed from the Python function, as it doesn't seem to be callable once the manager is initialized

@@ -163,3 +163,38 @@ void vine_set_runtime_info_path(const char *path)
assert(path);
vine_runtime_info_path = xxstrdup(path);
}

void vine_set_runtime_info_template(const char *dir)
Copy link
Member

Choose a reason for hiding this comment

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

Setting the template should only modify the value from "%Y-%m-%dT%H%M%S". vine_set_runtime_info_template should be called inside the other function to actually create the directories.

Copy link
Member Author

Choose a reason for hiding this comment

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

@btovar Though I was able to create the directories with the desired names, I assume it is because the vine_set_runtime_info_path and vine_set_runtime_info_template are called prior to vine_ssl_create in the manager's code, so the manager has the right global variables and env variables to initialize?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, we are setting the variables just before the manager creation.

@JinZhou5042
Copy link
Member Author

Looks great!

@btovar btovar merged commit 06e6b38 into cooperative-computing-lab:master Feb 6, 2025
9 of 10 checks passed
btovar added a commit that referenced this pull request Feb 6, 2025
* add run_info_dir

* vine: rename runtime_info_path to runtime_info_template

* vine: check if the template has exists

* lint

* set info template with api

* update python

* update param name

---------

Co-authored-by: Benjamin Tovar <[email protected]>
@JinZhou5042 JinZhou5042 deleted the rename_vine_log branch February 6, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vine: rename logs
3 participants