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

Bugfix/cwd flag closes #1160 #1161

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Conversation

dimidagd
Copy link
Contributor

@dimidagd dimidagd commented Nov 21, 2023

closes #1160 # A bug was preventing the use of --cwd and --folder. More specifically a function call to Path(path1).relative_to(path2) , where path1 has been stripped of the trailing directories, resulted in a specific exception being raised.

How to test fix

Given a tree structure

project_root
        ├── notebooks
        │   ├── run_clearml.py

Running

clearml-task --project testing --name test_run --script notebooks/run_clearml.py --task-type testing  --folder $PWD --queue default --cwd notebooks

Expected behaviour

Creating new task
Repository Detected
{
  "repository": "###",
  "version_num": "5946cd78e5a69e0a6916af8fb734c907de5feace",
  "branch": "main",
  "working_dir": "notebooks",
  "entry_point": "run_clearml.py"
}

New task created id=9a140105541a40478b8e0dac87317d2b

@dimidagd dimidagd changed the title Bugfix/cwd flag Fixes #1160 Bugfix/cwd flag closes #1160 Nov 21, 2023
@dimidagd dimidagd marked this pull request as draft November 21, 2023 16:46
@eugen-ajechiloae-clearml
Copy link
Collaborator

eugen-ajechiloae-clearml commented Jan 28, 2024

Hi @dimidagd ! I think this is a good change, but can you please make some changes such that the section you changed is a bit clearer? It's a bit hard to follow. You could add some comments that exemplifies what is going on in this section.

Also, looks like there are some conflicts, can you resolve those please?

@jkhenning
Copy link
Member

@dimidagd any update on this?

@dimidagd
Copy link
Contributor Author

dimidagd commented Feb 1, 2024 via email

@eugen-ajechiloae-clearml
Copy link
Collaborator

@dimidagd yes, the section itself (even excluding the changes) is a bit hard to read. Can you please add a few comments that could make it a bit easier to follow?

@@ -218,15 +215,14 @@ def create_task(self, dry_run=False):
task_state['script']['requirements'] = repo_info.script.get('requirements') or {}
if self.cwd:
self.cwd = self.cwd
cwd = self.cwd if Path(self.cwd).is_dir() else (
Path(repo_info.script['repo_root']) / self.cwd).as_posix()
cwd = ( Path(repo_info.script['repo_root']) / self.cwd).as_posix()
Copy link
Contributor Author

@dimidagd dimidagd Feb 2, 2024

Choose a reason for hiding this comment

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

Initialize only to to absolute path

entry_point = \
Path(repo_info.script['repo_root']) / repo_info.script['working_dir'] / repo_info.script[
'entry_point']
entry_point = entry_point.relative_to(cwd).as_posix()
cwd = Path(cwd).relative_to(repo_info.script['repo_root']).as_posix()
Copy link
Contributor Author

@dimidagd dimidagd Feb 2, 2024

Choose a reason for hiding this comment

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

cwd can be replaced with relative path 'to cwd from entry_point '

if not Path(cwd).is_dir():
raise ValueError("Working directory \'{}\' could not be found".format(cwd))
cwd = Path(cwd).relative_to(repo_info.script['repo_root']).as_posix()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move cwd override by relative path later on, because entry_point has to be resolved

@dimidagd
Copy link
Contributor Author

dimidagd commented Feb 2, 2024

@dimidagd yes, the section itself (even excluding the changes) is a bit hard to read. Can you please add a few comments that could make it a bit easier to follow?

Have added comments now.

@jkhenning
Copy link
Member

@dimidagd it seems there are conflicts - can you please resolve them?

@dimidagd dimidagd marked this pull request as ready for review February 9, 2024 11:25
@jkhenning jkhenning merged commit 956e7e4 into allegroai:master Feb 9, 2024
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.

Bug when --cwd --folder and --script are given.
3 participants