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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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!

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