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

Introduce {cont_dspath} to contain what {img_dspath} was and fix {img_dspath} #201

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

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Mar 8, 2023

img_dspath to point to actual ds containing the img. Despite description in the containers-add saying that it points to the dataset containing the image, it is not necessarily the case whenever images are contained in a subdataset of the dataset which carries definitions of the container. So, this PR fixes that and also adds cont_dspath which would then contain relative path to the dataset containing the definition of the container. This way it would be possible to enable both cases - referencing commands within subdataset containing image or containing the definition of the container.

It came up in the context of ReproNim/containers#86 where I am thinking to copy definition of containers into super-dataset so it would make it easy to "freeze" them in superdataset while leaving subdataset with actual container definitions without modifications.

TODOs:

  • add formatting of img_dspath etc based on cont_dspath.
  • get away from naive get_dataset_root to logic which would ensure first that we have installed all subdatasets leading to that location first since otherwise we simply cannot know the sub dataset actually containing the image.

…_dspath to point to actual ds containing the img

TODOs: add formatting of img_dspath etc based on cont_dspath.

For now committing/testing as is to see if breaks anything (should not)
@codeclimate
Copy link

codeclimate bot commented Mar 8, 2023

Code Climate has analyzed commit 7078de3 and detected 0 issues on this pull request.

View more on Code Climate.

Note that description of {img_*}  placeholders did not have to change
at all -- so to some degree the changes actually fixed it to become
corresponding with the description.
Because then we would need to format in other places like for get etc,
not to mention that likely in reproman would need to do more magic.
Since it makes sense ATM only to have path from the location where container
is defined, just keep it that way and not bother interpolating.
@yarikoptic yarikoptic added minor Increment the minor version when merged CHANGELOG-missing labels Mar 9, 2023
@yarikoptic yarikoptic changed the title WiP: introduce cont_dspath to contain what img_dspath was and fix img_dspath Introduce {cont_dspath} to contain what {img_dspath} was and fix {img_dspath} Mar 9, 2023
@yarikoptic
Copy link
Member Author

yarikoptic commented Mar 9, 2023

travis is green but appveyor reported following which seems generic absence of singularity - ie unrelated
=========================================================================================================== short test summary info ============================================================================================================
FAILED ../tests/test_run.py::test_container_files - datalad.support.exceptions.IncompleteResultsError: Command did not complete successfully. 1 failed:
[{'action': 'run',
  'exception': CommandError(''),
  'exception_traceback': '[run.py:_execute_command:691,runner.py:run:223]',
  'exit_code': 127,
  'message': 'singularity exec righthere ls',
  'msg_path': '/home/appveyor/DLTMP/datalad_temp_test_container_files80adhv_p/.git/COMMIT_EDITMSG',
  'path': '/home/appveyor/DLTMP/datalad_temp_test_container_files80adhv_p',
  'run_info': {'chain': [],
               'cmd': 'singularity exec righthere ls',
               'dsid': '41d24fd0-73ab-45a5-a49a-5e39d121abee',
               'exit': 127,
               'extra_inputs': ['righthere'],
               'inputs': [],
               'outputs': [],
               'pwd': '.'},
  'status': 'error',
  'type': 'dataset'}]
FAILED ../tests/test_run.py::test_run_no_explicit_dataset - datalad.support.exceptions.IncompleteResultsError: Command did not complete successfully. 1 failed:
[{'action': 'run',
  'exception': CommandError(''),
  'exception_traceback': '[run.py:_execute_command:691,runner.py:run:223]',
  'exit_code': 127,
  'message': 'singularity exec .datalad/environments/d...',
  'msg_path': '/home/appveyor/DLTMP/datalad_temp_tree_test_run_no_explicit_datasetvjow90c1/.git/COMMIT_EDITMSG',
  'path': '/home/appveyor/DLTMP/datalad_temp_tree_test_run_no_explicit_datasetvjow90c1',
  'run_info': {'chain': [],
               'cmd': 'singularity exec .datalad/environments/deb/image cat '
                      '{inputs[0]} {inputs[0]} >doubled',
               'dsid': 'd6ecd6a4-d716-4f5c-b5f4-1913bc06787d',
               'exit': 127,
               'extra_inputs': ['.datalad/environments/deb/image'],
               'inputs': ['subdir/in'],
               'outputs': ['doubled'],
               'pwd': '.'},
  'status': 'error',
  'type': 'dataset'}]
FAILED ../tests/test_schemes.py::test_docker - FileNotFoundError: [Errno 2] No such file or directory: 'singularity'

appveyor is also red in #200 so - must be unrelated

image_path = container["path"]
# container definition might point to an image in some nested dataset.
# it might be useful to be distinguish between the two in such cases
image_dspath = op.relpath(get_dataset_root(image_path), pwd)
Copy link
Member Author

Choose a reason for hiding this comment

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

get_dataset_root is likely not enough here! We must first ensure that there is a subdataset there, so pretty much perform the same trill as datalad get does to install all subdatasets leading to that path. Will need to check for that later and add a dedicated test for that to have hope to get this PR "proper".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant