Skip to content

Conversation

@wljungbergh
Copy link
Member

No description provided.

),
):
# we have to make sure aria2c is installed
check_aria_install()
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice if this check considered the dry_run flag. So that I could see what command would run even without having aria installed :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. Added!



@app.command()
def download(
Copy link
Collaborator

Choose a reason for hiding this comment

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

almost everything here is shared with the dropbox download script. Can we have a single, shared, entrypoint to simplify maintenance?
something like:
zod download --torrent (with interactive question if not specified)
or hierarchical like
zod download torrent vs zod download dropbox (but maybe messy to share arguments then?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it to be an argument in the download script, acc. your first suggestion. Now we have an entrypoint that chooses what downloading script to run based on user choice.

subprocess.check_call(["aria2c", "--version"])
except FileNotFoundError:
print(
"aria2c is not installed. Please install aria2c to download the dataset. Using: `apt install aria2`" # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice to provide the user with a browser link to academic torrents, and a message stating that you don't have to install aria2 and can instead download and extract the dataset yourself with your chosen torrent client.
Bonus points if we tell the user which files to download depending on the provided flags :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Dont think adding the file indices is useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we have some hardcoded things like torrent paths and file sizes here, that could potentially become outdated in the future. How about we just print a generic message if something goes wrong, along the lines of:
'this and this went wrong. please make sure to update to the latest cli version. if this doesn't fix it, please submit a github issue. ciao bye'

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this where it applies.

from enum import Enum

from zod.cli.dropbox_download import DownloadSettings, FilterSettings
from zod.cli.dropbox_download import download_dataset as drobbox_download
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from zod.cli.dropbox_download import download_dataset as drobbox_download
from zod.cli.dropbox_download import download_dataset as dropbox_download

_download_dataset(download_settings, filter_settings, subset.folder)

if source == DownloadSource.DROPBOX:
drobbox_download(download_settings, filter_settings, subset.folder)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
drobbox_download(download_settings, filter_settings, subset.folder)
dropbox_download(download_settings, filter_settings, subset.folder)

@Petros626
Copy link

still a problem: #68

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.

5 participants