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

Expand download options #928

Merged
merged 18 commits into from
Mar 1, 2022
Merged

Expand download options #928

merged 18 commits into from
Mar 1, 2022

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Feb 24, 2022

Close #926 by implementing new behaviors for mapdl.download:

mapdl.download('all')  # All simulation files, meaning files ending in 'out', 'full', 'rst', 'cdb', 'err', 'db', or 'log'
mapdl.download('everything')  # **Every single file** in the working directory.
mapdl.download('*.log')  # Every file which extension is 'log'. Uses global pattern.
mapdl.download('file.txt')  # Download 'file.txt'. Standard used behavior.
mapdl.download(['file0.txt', 'file0.rst']) # Download a list of files. Accept list of strings or tuple of strings.

Notes

  • Sure the labels could be improved. 'all' and 'everything' seems repetitive. But 'all' is a standard so I don't think it should include every single file in the MAPDL working directory. There many files there... batch, esav, mode, etc which I don't think will be useful most of the time.

Happy to have any kind of feedback.

@germa89
Copy link
Collaborator Author

germa89 commented Feb 24, 2022

@akaszynski can you check on linux locally?

@akaszynski
Copy link
Collaborator

@akaszynski can you check on linux locally?

Will do.

@nmalsang
Copy link

@germa89 For the labels we could replace:

  • "all" by "project"
  • "everything" by "working directory"

nmalsang
nmalsang previously approved these changes Feb 25, 2022

"""

extensions_all = ['OUT', 'FULL', 'RST', 'CDB', 'ERR', 'DB', 'LOG']

Choose a reason for hiding this comment

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

@germa89 From my remembers when I was a solver developer, there are more file extensions existing for mapdl as : .rsdp .rfrq .mode .rth .rmg .dump .....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, but those are the files extension that will be downloaded when issuing mapdl.download('all'). I believe most of the time, the regular user is interested mainly in ['OUT', 'FULL', 'RST', 'CDB', 'ERR', 'DB', 'LOG'].

Copy link
Collaborator

@akaszynski akaszynski Feb 25, 2022

Choose a reason for hiding this comment

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

We need a download_project. Prototype:

def download_project(self, extensions=['rst', 'full', <more>]):

Remove '"project"' and '"all"fromdownload. The files parameter should accept either a list or a glob` pattern, but not general terms like "all" and "project". Edge case is someone wants to download a file named "all", but it seems like a bad design practice to have a non-explicit API.

@akaszynski
Copy link
Collaborator

akaszynski commented Feb 25, 2022

I'd like to see the following:

  • Aforementioned download_project noted in Expand download options #928 (comment)
  • "global pattern" --> "glob pattern"
  • Remove "all" and "project" from download. Methods and parameters should be explicit.
  • Softly deprecate target with a warning and then raise an error for multiple files. Add in target_dir as this makes sense for multiple files.

@akaszynski akaszynski changed the title Expanding download options Expand download options Feb 25, 2022
@akaszynski akaszynski dismissed nmalsang’s stale review February 25, 2022 10:32

API needs to be fixed

@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #928 (9459ef7) into main (9f9128f) will increase coverage by 0.04%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
+ Coverage   72.78%   72.83%   +0.04%     
==========================================
  Files          39       39              
  Lines        5681     5676       -5     
==========================================
- Hits         4135     4134       -1     
+ Misses       1546     1542       -4     

@germa89
Copy link
Collaborator Author

germa89 commented Feb 25, 2022

Since the download function is fully changed, I did not implement a "soft deprecation" of "target".

If you want, I can add a keyword "target", that if provided, will raise that warning. We would change the exception (input a keyword that is not accepted) by a warning. But I don't see a good reason for that, and I don't like it.

Btw, I changed in source code any call to the old 'download' function for '_download'. Hence we should not see more errors.

Basically, if you want the old function, use mapdl._download(). And for downloading multiple files use the new mapdl.download().
To download the whole working directory, use mapdl.download_project().

All of this is properly documented in our docs (guide + docstrings).

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.

Have endpoints on each products APIs to upload and to download easily the user data
3 participants