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

App: implement managing multiple projects (#107) #109

Merged
merged 38 commits into from
Dec 20, 2019

Conversation

keggsmurph21
Copy link
Collaborator

See issue #107 for more details

@keggsmurph21 keggsmurph21 added the enhancement New feature or request label Dec 18, 2019
Copy link
Collaborator

@calebho calebho left a comment

Choose a reason for hiding this comment

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

Tests? :)

ultratrace2/app.py Outdated Show resolved Hide resolved
ultratrace2/app.py Outdated Show resolved Hide resolved
ultratrace2/model/project.py Outdated Show resolved Hide resolved
ultratrace2/model/files/bundle.py Outdated Show resolved Hide resolved
Following feedback on #107, this patch implements saving the ultratrace
metadata directly in the directory containing the source files.  In
addition, it changes the way we construct Projects and FileBundleLists
so that we avoid half-constructed objects.
@keggsmurph21
Copy link
Collaborator Author

Next up: tests!

ultratrace2/model/files/bundle.py Outdated Show resolved Hide resolved
ultratrace2/model/files/bundle.py Outdated Show resolved Hide resolved
ultratrace2/model/files/bundle.py Show resolved Hide resolved
ultratrace2/model/files/bundle.py Outdated Show resolved Hide resolved
ultratrace2/model/project.py Outdated Show resolved Hide resolved
ultratrace2/model/project.py Show resolved Hide resolved
ultratrace2/model/project.py Outdated Show resolved Hide resolved
ultratrace2/model/project.py Outdated Show resolved Hide resolved
ultratrace2/model/project.py Outdated Show resolved Hide resolved
ultratrace2/model/project.py Outdated Show resolved Hide resolved
ultratrace2/app.py Outdated Show resolved Hide resolved
NB: These tests only test for INVALID input. There are currently no
    tests for VALID input (since that behavior is not defined yet).
Still unresolved:
 - How can we specify the desired resolution order (and still allow it
   be flexible / extensible)?
Python already includes a standard library for working with mimetypes,
so it's unclear why we were ever using `python-magic`.
def set_alignment_file(self, alignment_file: AlignmentFile) -> None:
def set_alignment_file(self, alignment_file: AlignmentFileLoader) -> None:
if self.alignment_file is not None:
logger.warning("Overwriting existing alignment file")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@calebho This is another reason why we might want some kind of resolution order. Consider the case where we're scanning a directory that contains XYZ.wav and XYZ.mp3 (assuming we know how to handle both). Clearly, we would prefer the losslessly encoded wav file, but under the current system he one we "end up with" depends only on the order of directory traversal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Clearly, we would prefer the losslessly encoded wav file

Are you sure that this matches the user's preference? If all users always have this preference, why would they include the recording in both encodings?

I'm not sure how this relates to having more than one loader for a given extension. Perhaps I'm misunderstanding what you mean by "resolution order".

Copy link
Collaborator

Choose a reason for hiding this comment

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

A user might have a lossy file around for easy playback / demonstration purposes.

For a real example of something similar, I usually use flac for things like this (lossless compression), but might have wavs around because that's what the original data was recorded in and I don't like keeping the original around. (The choice between wav and flac would just be one of resources: speed (wav better?) or memory (flac better?).)

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'm not sure how this relates to having more than one loader for a given extension. Perhaps I'm misunderstanding what you mean by "resolution order".

So by "resolution order", I mean two separate things:

  1. As Jonathan mentioned, $user has files XYZ.flac and XYZ.wav:

    • As we traverse the directory, we encounter both.
    • WLOG, let's say we encounter XYZ.flac first.
    • We retrieve FLACFileLoader from the registry and successfully load XYZ.flac.
    • We save it to bundles["xyz"].sound_file.
    • We continue traversing, and we encounter XYZ.wav.
    • We retrieve WAVFileLoader from the registry and succesfully load XYZ.flac.
    • We go to save it to bundles["xyz"].sound_file, but that's already been set.
    • Currently, we just print a logger.warning and overwrite it, but we might want to give certain loaders "preference" over others, so that we might always choose FLAC, for example.
  2. $user has the file ABC.flac and registers MyFLACLoader with mime types ["audio/x-flac", "audio/flac"] and extensions ["flac"].

    • We traverse the directory and encounter ABC.flac.
    • We try to retrieve from the registry, but error out since it found two possible loaders, but, again, we might want to give certain loaders "preference" over others (presumably user-defined ones should be preferred, but then what happens if they try to register MyOtherFLACLoader with the same mime types / extensions?).

I think (1) is a real possibility, and (2) is also possible but less likely. One solution I can see is to present the user with a dialog for choosing their "FileBundle associations" (i.e., which sound file corresponds to which alignment file corresponds to which image-set file) in the case of conflicts, but to resolve automatically where possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, we just print a logger.warning and overwrite it, but we might want to give certain loaders "preference" over others, so that we might always choose FLAC, for example.

Ditto. For now I think it makes sense to hard code this preference, then expose it as a user configurable thing later.

We try to retrieve from the registry, but error out since it found two possible loaders

Does it make sense to differentiate an audio/x-flac FLAC from an audio/flac FLAC? Aren't they the same?

we might want to give certain loaders "preference" over others (presumably user-defined ones should be preferred, but then what happens if they try to register MyOtherFLACLoader with the same mime types / extensions?)

Does it make sense to have multiple loaders registered at once? We can have the notion of preference without keeping all possible preferences loaded at the same time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, we just print a logger.warning and overwrite it, but we might want to give certain loaders "preference" over others, so that we might always choose FLAC, for example.

Ditto. For now I think it makes sense to hard code this preference, then expose it as a user configurable thing later.

OK, at what level in the abstraction would be enforce that? Give each FileLoader a priority class attribute and take the highest?

We try to retrieve from the registry, but error out since it found two possible loaders

Does it make sense to differentiate an audio/x-flac FLAC from an audio/flac FLAC? Aren't they the same?

Yes, they are equivalent. But that's not the reason it found two ... each file only has one mime type. So WLOG, ("audio/x-flac", "flac") could map to FLACLoader (built-in) and MyFLACLoader (user extension).

we might want to give certain loaders "preference" over others (presumably user-defined ones should be preferred, but then what happens if they try to register MyOtherFLACLoader with the same mime types / extensions?)

Does it make sense to have multiple loaders registered at once? We can have the notion of preference without keeping all possible preferences loaded at the same time.

I think probably it does not make sense. We could emit a logger.warning if we're overwriting an existing FileLoader at register-time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to differentiate an audio/x-flac FLAC from an audio/flac FLAC? Aren't they the same?

Ftr, the reason we have both in there is because we were getting files identified as either. I forget if it was platform-specific or file-specific, but we definitely needed it to respond to either.

ultratrace2/model/files/loaders/base.py Outdated Show resolved Hide resolved
ultratrace2/model/files/loaders/base.py Outdated Show resolved Hide resolved
ultratrace2/model/files/loaders/base.py Outdated Show resolved Hide resolved
ultratrace2/model/files/loaders/base.py Outdated Show resolved Hide resolved
ultratrace2/model/files/loaders/dicom.py Outdated Show resolved Hide resolved
ultratrace2/model/files/registry.py Outdated Show resolved Hide resolved
keggsmurph21 and others added 6 commits December 19, 2019 17:53
Apparently, `global` is only necessary for assignment, and these
functions only need to access the value.
Instead, let the exceptions propagate up to the caller.  For one thing,
this allows us to write more specific tests :^)
@keggsmurph21 keggsmurph21 force-pushed the manage-multiple-projects branch from 4f86115 to e6895ba Compare December 20, 2019 02:40
@keggsmurph21 keggsmurph21 force-pushed the manage-multiple-projects branch from ceb0642 to 2669b54 Compare December 20, 2019 03:08
@keggsmurph21 keggsmurph21 merged commit 34492f9 into master Dec 20, 2019
@keggsmurph21 keggsmurph21 deleted the manage-multiple-projects branch December 20, 2019 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants