Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

one upload command per folder. no default ignore statement. doesn't w… #7

Merged
merged 5 commits into from
Jan 18, 2019

Conversation

aledj2
Copy link
Contributor

@aledj2 aledj2 commented Jan 16, 2019

…rite to stderr. fix #5 fix #4


This change is Reviewable

@aledj2
Copy link
Contributor Author

aledj2 commented Jan 16, 2019

fixes #8

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @andyb3)


backup_runfolder.py, line 219 at r1 (raw file):

        
        # Return the nexus folder and full project filepath
        return nexus_path, "{}:{}".format(self.project, nexus_path)

FYI just out of interest I recently learned about Python3 f-strings, which is essentially like .format() but you instead just prefix the string with f and then insert your variables or whatever directly into the braces, so here you could do:
return nexus_path, f"{self.project}:{nexus_path}"

https://realpython.com/python-f-strings/#f-strings-a-new-and-improved-way-to-format-strings-in-python

Copy link
Contributor Author

@aledj2 aledj2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @andyb3)


backup_runfolder.py, line 219 at r1 (raw file):

Previously, andyb3 wrote…

FYI just out of interest I recently learned about Python3 f-strings, which is essentially like .format() but you instead just prefix the string with f and then insert your variables or whatever directly into the braces, so here you could do:
return nexus_path, f"{self.project}:{nexus_path}"

https://realpython.com/python-f-strings/#f-strings-a-new-and-improved-way-to-format-strings-in-python

thats cool! will add that as an issue on the repo for future developments

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @aledj2 and @andyb3)


backup_runfolder.py, line 294 at r1 (raw file):

                files_string = ""
                for file in file_dict[path]:
                    files_string = files_string + " '" + os.path.join(path, file) + "'"

upload agent can upload max 1000 files in single operation. Could we have folders that have more than 1000 files?

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aledj2)

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aledj2)


backup_runfolder.py, line 293 at r2 (raw file):

                self.logger.info('Calling upload agent on %s to location %s', path, project_filepath)
                # upload agent has a max number of uploads of 1000 per command
                # count number of files in list and divide by 1000.0 eg 20/1000.0 = 0.02 (need to make the denominator this a float as 1200/1000 = 1 but 1200/1000.0 = 1.2) . ceil rounds up to the nearest integer (0.02->1). If there are 1000, ceil(1000/1000.0)=1

This actually isn't true in Python3. Floating point division is now default.


backup_runfolder.py, line 315 at r2 (raw file):

                    iteration_count += 1
                    start += 1000
                    stop += 1000

FYI if you take a slice that is longer than the list, you don't get an error, it just returns what is there, so don't think it is necessary to calculate how many iterations you need etc., I think you can just loop through taking slices of 1000 until there's nothing left in the list (although if you're writing the 'uploading file %d to %d' bit to log file you do still need to keep count of starts and stops).

Something like this...

# Record start and stop so that they can written to logfile
start = 1
stop = len(file_dict[path][:1000])
# Whilst there are still files to be uploaded...
while file_dict[path]:
    # Take first 1000 files from list and create string for upload command
    file_string = "'" + "' '".join(file_dict[path][:1000]) + "'"
    self.logger.info('uploading files %d to %d', start, stop)
    # Issue upload command etc.
    nexus_upload_command = ....
    ....
    # Remove uploaded files from list
    del file_dict[path][:1000]
    # Update start and stop values for log files ready for next iteration
    start = stop + 1
    stop = start + (len(file_dict[path][:1000]) - 1)

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aledj2)


backup_runfolder.py, line 315 at r2 (raw file):

Previously, andyb3 wrote…

FYI if you take a slice that is longer than the list, you don't get an error, it just returns what is there, so don't think it is necessary to calculate how many iterations you need etc., I think you can just loop through taking slices of 1000 until there's nothing left in the list (although if you're writing the 'uploading file %d to %d' bit to log file you do still need to keep count of starts and stops).

Something like this...

# Record start and stop so that they can written to logfile
start = 1
stop = len(file_dict[path][:1000])
# Whilst there are still files to be uploaded...
while file_dict[path]:
    # Take first 1000 files from list and create string for upload command
    file_string = "'" + "' '".join(file_dict[path][:1000]) + "'"
    self.logger.info('uploading files %d to %d', start, stop)
    # Issue upload command etc.
    nexus_upload_command = ....
    ....
    # Remove uploaded files from list
    del file_dict[path][:1000]
    # Update start and stop values for log files ready for next iteration
    start = stop + 1
    stop = start + (len(file_dict[path][:1000]) - 1)

Actaully that doesn't do the os.path.join() bit on the files so would need changing. Not actually sure it's any simpler than what you've already done so please ignore me!

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aledj2)


backup_runfolder.py, line 294 at r1 (raw file):

Previously, andyb3 wrote…

upload agent can upload max 1000 files in single operation. Could we have folders that have more than 1000 files?

LGTM

Copy link
Contributor Author

@aledj2 aledj2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andyb3)


backup_runfolder.py, line 293 at r2 (raw file):

Previously, andyb3 wrote…

This actually isn't true in Python3. Floating point division is now default.

will change

Copy link
Contributor Author

@aledj2 aledj2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @andyb3)


backup_runfolder.py, line 293 at r2 (raw file):

Previously, aledj2 (Aled Jones) wrote…

will change

Done.

Copy link
Contributor

@andyb3 andyb3 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@andyb3 andyb3 merged commit 1c84e8a into master Jan 18, 2019
@andyb3 andyb3 deleted the v1.1 branch January 18, 2019 14:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove default statement for --ignore Does it need to print log to stdout as well as writing to file?
2 participants