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

ENH: Enable progress bars for unlock, fixes #6691 #6704

Merged
merged 1 commit into from
May 31, 2022

Conversation

adswa
Copy link
Member

@adswa adswa commented May 24, 2022

An unlocking operation can take a considerable amount of
time, but doesn't provide feedback at all (see #6691).
By switching from _call_annex_records(), which processes
the full bunch of files at once, to _call_annex_records_items(),
which yields as soon as results are available, we can
construct a progress bar with a file counter around the
unlock operation.

The progress reporting is slightly complicated by the
fact that git annex' --json records for each file are
output before a final 'recording state in git' operations,
which can, if a large number of files was unlocked, add
considerably to the execution time of the command after
all files have been unlocked already and the progress bar
would be at 100%. Thus, this change adds an additional
progress update when all files are processed to report
on the final recording step.

Nevertheless, the progress bar seems a bit moody - it seems that the _call_annex_records_items() loop doesn't yield all files' results before the "recording state in git" stage, leading to a bit of a pause in the middle of the progress bar. Maybe @christian-monch knows what might cause this?
unlock-progressbar

Another TODO/Weirdness is a for some reason completely made up file counter (you can see it in the reported file total in the Gif as well) that I wasn't able to figure out. Every run reports a different wrong total number of files, when the total being declared in the setup of the progressbar (nfiles) is actually correct.

Reproducer:

datalad create abcd && abcd
for i in {1..1000}; do echo $i > file_$i ; done
datalad save
datalad unlock 

I think I have seen similar flaky reporting in other commands (push?) so I suspect its something more general with the progress bar setup.

Changelog

💫 Enhancements and new features

  • datalad unlock gained a progress bar.

@adswa adswa added the semver-patch Increment the patch version when merged label May 24, 2022
@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #6704 (01d2619) into master (c21a31c) will increase coverage by 1.59%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6704      +/-   ##
==========================================
+ Coverage   88.81%   90.40%   +1.59%     
==========================================
  Files         353      353              
  Lines       45733    45742       +9     
==========================================
+ Hits        40617    41353     +736     
+ Misses       5116     4389     -727     
Impacted Files Coverage Δ
datalad/local/unlock.py 100.00% <100.00%> (ø)
datalad/__init__.py 97.95% <0.00%> (+16.32%) ⬆️
datalad/tests/utils.py 57.11% <0.00%> (+35.48%) ⬆️
datalad/tests/test_tests_utils.py 92.39% <0.00%> (+92.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c21a31c...01d2619. Read the comment docs.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

Changes like this need to be done all over the code base.

If by 'total counter' you refer to the larger number on the right, it us not a total but a speed estimate. I agree that it is generally pretty useless and inaccurate, but technically not wrong.

@adswa
Copy link
Member Author

adswa commented May 25, 2022

If by 'total counter' you refer to the larger number on the right, it us not a total but a speed estimate. I agree that it is generally pretty useless and inaccurate, but technically not wrong.

oh sorry 🤦

An unlocking operation can take a considerable amount of
time, but doesn't provide feedback at all (see datalad#6691).
By switching from _call_annex_records(), which processes
the full bunch of files at once, to _call_annex_records_items(),
which yields as soon as results are available, we can
construct a progress bar with a file counter around the
unlock operation.

The progress reporting is slightly complicated by the
fact that git annex' --json records for each file are
output before a final 'recording state in git' operations,
which can, if a large number of files was unlocked, add
considerably to the execution time of the command after
all files have been unlocked already and the progress bar
would be at 100%. Thus, this change adds an additional
progress update when all files are processed to report
on the final recording step.
@codeclimate
Copy link

codeclimate bot commented May 25, 2022

Code Climate has analyzed commit 01d2619 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

LGTM. Curious CI failures, though. They seem unrelated, but I haven't seen them before.

@adswa
Copy link
Member Author

adswa commented May 25, 2022

I've seen them in a few previous PRs before (e.g., #6701, #6698) but am also unsure what to make if them as well

@mih
Copy link
Member

mih commented May 25, 2022

datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmodules=none -c annex.merge-annex-branches=false annex find --not --in . --json --json-error-messages -c annex.dotfiles=true -- file1' failed with exitcode 1 under /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_ro_operationsa5gnt_3r/clone [info keys: stdout_json] [err: 'git-annex: This repository is not initialized for use by git-annex, but .git/annex/objects/ exists, which indicates this repository was used by git-annex before, and may have lost its annex.uuid and annex.version configs. Either set back missing configs, or run git-annex init to initialize with a new uuid.']

Posting this error here in order to help find this PR later. But if this has anything to do with this PR, it would be an indication of a runner issue. If there is such an issue, the runner would need to be fixed. Because AFAICS the usage pattern in this PR is pretty normal. Maybe @christian-monch can comment.

@christian-monch
Copy link
Contributor

Nevertheless, the progress bar seems a bit moody - it seems that the _call_annex_records_items() loop doesn't yield all files' results before the "recording state in git" stage, leading to a bit of a pause in the middle of the progress bar. Maybe @christian-monch knows what might cause this?

Sorry, but I cannot reproduce the pause locally. Let's try to figure out why datalad unlock behaves so differently.

I do see a strange behavior in datalad save after datalad unlock though. It takes about a minute to complete.

@christian-monch
Copy link
Contributor

Maybe @christian-monch can comment.
Looking into the reasons for the CommandError.

@adswa
Copy link
Member Author

adswa commented May 30, 2022

Sorry, but I cannot reproduce the pause locally. Let's try to figure out why datalad unlock behaves so differently.

I do see a strange behavior in datalad save after datalad unlock though. It takes about a minute to complete.

Thanks for looking into this! I don't see anything unusual in save - its rapid. Might suggest something the takes place during unlock for me happens during your save?
Here's my wtf, do you see what might be different from yours?

❱ datalad wtf
# WTF
## configuration <SENSITIVE, report disabled by configuration>
## credentials 
  - keyring: 
    - active_backends: 
      - SecretService Keyring
      - PlaintextKeyring with no encyption v.1.0 at /home/adina/.local/share/python_keyring/keyring_pass.cfg
    - config_file: /home/adina/.config/python_keyring/keyringrc.cfg
    - data_root: /home/adina/.local/share/python_keyring
## datalad 
  - version: 0.16.2+181.g6bfe67007
## dependencies 
  - annexremote: 1.6.0
  - boto: 2.49.0
  - cmd:7z: 16.02
  - cmd:annex: 10.20220504
  - cmd:bundled-git: UNKNOWN
  - cmd:git: 2.35.2
  - cmd:system-git: 2.35.2
  - cmd:system-ssh: 8.4p1
  - exifread: 2.3.2
  - git: 3.1.27
  - gitdb: 4.0.9
  - humanize: 4.0.0
  - iso8601: 1.0.2
  - keyring: 23.5.0
  - keyrings.alt: 4.1.0
  - msgpack: 1.0.3
  - mutagen: 1.45.1
  - platformdirs: 2.5.1
  - requests: 2.27.1
## environment 
  - LANG: en_US.UTF-8
  - LANGUAGE: en_US.UTF-8
  - LC_ALL: en_US.UTF-8
  - LC_CTYPE: en_US.UTF-8
  - PATH: /home/adina/env/handbook/bin:/home/adina/.dotfiles/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/X11R6/bin:/usr/local/games:/usr/games:/home/adina/.local/bin:/home/adina/bin:/snap/bin:/home/adina/repos/latexrun/:/home/adina/node_modules/.bin
## extensions 
  - container: 
    - description: Containerized environments
    - entrypoints: 
      - datalad_container.containers_add.ContainersAdd: 
        - class: ContainersAdd
        - load_error: None
        - module: datalad_container.containers_add
        - names: 
          - containers-add
          - containers_add
      - datalad_container.containers_list.ContainersList: 
        - class: ContainersList
        - load_error: None
        - module: datalad_container.containers_list
        - names: 
          - containers-list
          - containers_list
      - datalad_container.containers_remove.ContainersRemove: 
        - class: ContainersRemove
        - load_error: None
        - module: datalad_container.containers_remove
        - names: 
          - containers-remove
          - containers_remove
      - datalad_container.containers_run.ContainersRun: 
        - class: ContainersRun
        - load_error: None
        - module: datalad_container.containers_run
        - names: 
          - containers-run
          - containers_run
    - load_error: None
    - module: datalad_container
    - version: 1.1.6
  - datalad_debian: 
    - description: DataLad for working with Debian packages
    - entrypoints: 
      - datalad_debian.new_distribution.NewDistribution: 
        - class: NewDistribution
        - load_error: None
        - module: datalad_debian.new_distribution
        - names: 
          - deb-new-distribution
          - deb_new_distribution
      - datalad_debian.new_package.NewPackage: 
        - class: NewPackage
        - load_error: None
        - module: datalad_debian.new_package
        - names: 
          - deb-new-package
          - deb_new_package
      - datalad_debian.update_distribution.UpdateDistribution: 
        - class: UpdateDistribution
        - load_error: None
        - module: datalad_debian.update_distribution
        - names: 
          - deb-update-distribution
          - deb_update_distribution
    - load_error: None
    - module: datalad_debian
    - version: 0+untagged.18.g54652cc.dirty
  - neuroimaging: 
    - description: Neuroimaging tools
    - entrypoints: 
      - datalad_neuroimaging.bids2scidata.BIDS2Scidata: 
        - class: BIDS2Scidata
        - load_error: None
        - module: datalad_neuroimaging.bids2scidata
        - names: 
          - bids2scidata
    - load_error: None
    - module: datalad_neuroimaging
    - version: 0.3.1
  - ukbiobank: 
    - description: UKBiobank dataset support
    - entrypoints: 
      - datalad_ukbiobank.init.Init: 
        - class: Init
        - load_error: None
        - module: datalad_ukbiobank.init
        - names: 
          - ukb-init
          - ukb_init
      - datalad_ukbiobank.update.Update: 
        - class: Update
        - load_error: None
        - module: datalad_ukbiobank.update
        - names: 
          - ukb-update
          - ukb_update
    - load_error: None
    - module: datalad_ukbiobank
    - version: 0.3.3
  - webapp: 
    - description: Generic web app support
    - entrypoints: 
      - datalad_webapp.WebApp: 
        - class: WebApp
        - load_error: None
        - module: datalad_webapp
        - names: 
          - webapp
          - webapp
    - load_error: None
    - module: datalad_webapp
    - version: 0.3
## git-annex 
  - build flags: 
    - Assistant
    - Webapp
    - Pairing
    - Inotify
    - DBus
    - DesktopNotify
    - TorrentParser
    - MagicMime
    - Feeds
    - Testsuite
    - S3
    - WebDAV
  - dependency versions: 
    - aws-0.22
    - bloomfilter-2.0.1.0
    - cryptonite-0.26
    - DAV-1.3.4
    - feed-1.3.0.1
    - ghc-8.8.4
    - http-client-0.6.4.1
    - persistent-sqlite-2.10.6.2
    - torrent-10000.1.1
    - uuid-1.3.13
    - yesod-1.6.1.0
  - key/value backends: 
    - SHA256E
    - SHA256
    - SHA512E
    - SHA512
    - SHA224E
    - SHA224
    - SHA384E
    - SHA384
    - SHA3_256E
    - SHA3_256
    - SHA3_512E
    - SHA3_512
    - SHA3_224E
    - SHA3_224
    - SHA3_384E
    - SHA3_384
    - SKEIN256E
    - SKEIN256
    - SKEIN512E
    - SKEIN512
    - BLAKE2B256E
    - BLAKE2B256
    - BLAKE2B512E
    - BLAKE2B512
    - BLAKE2B160E
    - BLAKE2B160
    - BLAKE2B224E
    - BLAKE2B224
    - BLAKE2B384E
    - BLAKE2B384
    - BLAKE2BP512E
    - BLAKE2BP512
    - BLAKE2S256E
    - BLAKE2S256
    - BLAKE2S160E
    - BLAKE2S160
    - BLAKE2S224E
    - BLAKE2S224
    - BLAKE2SP256E
    - BLAKE2SP256
    - BLAKE2SP224E
    - BLAKE2SP224
    - SHA1E
    - SHA1
    - MD5E
    - MD5
    - WORM
    - URL
    - X*
  - operating system: linux x86_64
  - remote types: 
    - git
    - gcrypt
    - p2p
    - S3
    - bup
    - directory
    - rsync
    - web
    - bittorrent
    - webdav
    - adb
    - tahoe
    - glacier
    - ddar
    - git-lfs
    - httpalso
    - borg
    - hook
    - external
  - supported repository versions: 
    - 8
    - 9
    - 10
  - upgrade supported from repository versions: 
    - 0
    - 1
    - 2
    - 3
    - 4
    - 5
    - 6
    - 7
    - 8
    - 9
    - 10
  - version: 10.20220504
## location 
  - path: /home/adina
  - type: directory
## metadata_extractors 
  - annex: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.annex
  - audio: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.audio
  - bids: 
    - distribution: datalad-neuroimaging 0.3.1
    - load_error: None
    - module: datalad_neuroimaging.extractors.bids
  - datacite: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.datacite
  - datalad_core: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.datalad_core
  - datalad_rfc822: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.datalad_rfc822
  - dicom: 
    - distribution: datalad-neuroimaging 0.3.1
    - load_error: None
    - module: datalad_neuroimaging.extractors.dicom
  - exif: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.exif
  - frictionless_datapackage: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.frictionless_datapackage
  - image: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.image
  - nidm: 
    - distribution: datalad-neuroimaging 0.3.1
    - load_error: None
    - module: datalad_neuroimaging.extractors.nidm
  - nifti1: 
    - distribution: datalad-neuroimaging 0.3.1
    - load_error: None
    - module: datalad_neuroimaging.extractors.nifti1
  - xmp: 
    - distribution: datalad 0.16.1+3.g682c9109b
    - load_error: None
    - module: datalad.metadata.extractors.xmp
## metadata_indexers 
## python 
  - implementation: CPython
  - version: 3.9.12
## system 
  - distribution: debian/11-updates/n/a
  - encoding: 
    - default: utf-8
    - filesystem: utf-8
    - locale.prefered: UTF-8
  - filesystem: 
    - CWD: 
      - max_pathlength: 4096
      - mount_opts: rw,relatime,errors=remount-ro
      - path: /home/adina
      - type: ext4
    - HOME: 
      - max_pathlength: 4096
      - mount_opts: rw,relatime,errors=remount-ro
      - path: /home/adina
      - type: ext4
    - TMP: 
      - max_pathlength: 4096
      - mount_opts: rw,relatime,errors=remount-ro
      - path: /tmp
      - type: ext4
  - max_path_length: 267
  - name: Linux
  - release: 5.16.0-3-amd64
  - type: posix
  - version: #1 SMP PREEMPT Debian 5.16.11-1 (2022-02-25)

@mslw
Copy link
Contributor

mslw commented May 30, 2022

datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmodules=none -c annex.merge-annex-branches=false annex find --not --in . --json --json-error-messages -c annex.dotfiles=true -- file1' failed with exitcode 1 under /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/datalad_temp_tree_test_ro_operationsa5gnt_3r/clone [info keys: stdout_json] [err: 'git-annex: This repository is not initialized for use by git-annex, but .git/annex/objects/ exists, which indicates this repository was used by git-annex before, and may have lost its annex.uuid and annex.version configs. Either set back missing configs, or run git-annex init to initialize with a new uuid.']

Posting this error here in order to help find this PR later. But if this has anything to do with this PR, it would be an indication of a runner issue.

This looks like it's caused by a change in Git behavior.

Not sure which test produced the error, but I ran into the same error message on our cluster when cloning a dataset owned by another user (as an input subdataset).

Note that the failing test on MacOS (brew) had git 2.36.1, while the successful one (snapshot) had 2.35.1

In addition to the issues pointed by @adswa , the error appears in a few others. To tie things together: it was succinctly summarized by @yarikoptic in this issue comment and #6708

The summary (for my presentation of the problem, not sure if appplicable here) is that Git >= 2.35.2 will refuse to operate on .git owned by someone else, unless the user explicitly configures the directory containing it as trusted with git config --global --add safe.directory <path>. This is due to safety reasons, to avoid situations where a malicious actor could create a .git directory in a shared location above a victim’s current working directory. Context and links which I found useful here: https://stackoverflow.com/a/72299780

@yarikoptic
Copy link
Member

FWIW, looks nice and clean to me, so let's proceed

@yarikoptic yarikoptic merged commit aa34666 into datalad:master May 31, 2022
Copy link

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

thanks @adswa!

@adswa adswa deleted the enh-unlock-pbs branch May 31, 2022 13:31
@mih mih mentioned this pull request Jul 1, 2022
40 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants