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

Update README.md #1

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Update README.md #1

wants to merge 8 commits into from

Conversation

unnir
Copy link
Collaborator

@unnir unnir commented Oct 17, 2023

The update includes:

  • Windows Application. Complete code for the Windows application. Compile it using pyInstaller (https://pyinstaller.org/en/stable/). Check the windows_app folder for more details.

  • 1D CNN Model Training. Full source code for training the 1D CNN model, runnable on Google Colab or locally. Refer to the Colab link in the README or directly access the Jupyter Notebook here.

@unnir
Copy link
Collaborator Author

unnir commented Oct 17, 2023

@sethaxen please give me access to the main branch.

@sethaxen
Copy link
Member

@unnir Okay, I've temporarily disabled branch protection rules, but let me know when the bulk of the code has been contributed, so I can re-enable it.

@unnir
Copy link
Collaborator Author

unnir commented Oct 17, 2023

Great, thanks!

windows_app/untitled.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This file is quite large, and It's probably better for us to distribute this in a different manner than bundled with the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Up to you guys. In general, this size is considered very small.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but it's also about hygiene. We prefer the weights be stored separately from the code. The idea is that at runtime, the user can provide the path to the weights. If they don't, then the code should check if the weights are present in a predefined location in the package, and if they're not, then they are automatically downloaded, using a method like this:

import requests
import hashlib
import logging
import os

def download_artifact(url: str, download_path: str, expected_hash: str=None, hash_algorithm: str='sha256', chunk_size: int=2**20):
    """
    Downloads an artifact from a given URL and optionally checks its hash.
    
    Parameters
    ----------
    url : str
        The URL of the artifact.
    download_path : str
        The path where the artifact will be downloaded.
    expected_hash : str, optional
        The expected hash of the artifact. (default: `None`)
    hash_algorithm : str, optional
        The hash algorithm to use. (default: 'sha256')
    chunk_size : int, optional
        The size of the chunks to use when downloading the artifact. (default: 2**20)
    
    Returns
    -------
    None
    
    Raises:
    - ValueError: If the computed hash does not match the expected hash.
    """
    response = requests.get(url, stream=True)
    response.raise_for_status()  # Check for any errors and raise an exception if found.
    
    hasher = hashlib.new(hash_algorithm)
    with open(download_path, 'wb') as file:
        for chunk in response.iter_content(chunk_size=chunk_size):
            file.write(chunk)
            hasher.update(chunk)
    
    computed_hash = hasher.hexdigest()
    if expected_hash:
        if computed_hash != expected_hash:
            os.remove(download_path)  # Delete the downloaded file if the hash does not match.
            raise ValueError(f'Hash mismatch: expected {expected_hash}, but got {computed_hash}.')
    else:
        logging.debug(f'Computed hash: {computed_hash}')

As for where to place the weights, a simple approach is to make a release in this repo, and add the weights as an artifact to the release. Then the URL of the artifact and the hash can be specified in the code in the next commit.

In terms of how to do this, I suggest you send us the weights now through another means (e-mail, Google Drive, etc), and I can take care of setting up the artifact. For code, I only ask for now that you add placeholder code to automatically download the weights to the right place once we have the URL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We prefer the weights be stored separately from the code.

This is already the case; the weights are not part of the package. A user has to select weights from the drive, so we do not need to recompile the app every time. This way, I can just send Sam new updated weights, and she can still use the app.

The idea of downloading the weights is cool, but what if a user has no internet connection?

Anyway, currently, I do not have access to a Windows machine, so I won't be able to test the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

We prefer the weights be stored separately from the code.

This is already the case; the weights are not part of the package. A user has to select weights from the drive, so we do not need to recompile the app every time. This way, I can just send Sam new updated weights, and she can still use the app.

Can you tell me a little more about this workflow then? I see there's a windows_app/model/weights.path documented in the readme, but no such file in the repo, but there is a model.path. If the weights are not part of the package, then what is this model.path file, and why is it included? When the user launches the app, must they have already downloaded weights from the drive, or is there then an interface to do that? I also don't have a windows machine to test it for myself.

RE the windows app, how keyed to windows is it? Is there a way it could be run on other platforms?

The idea of downloading the weights is cool, but what if a user has no internet connection?

Then if weights are unavailable, an error would be thrown instructing the user how to get the weights. How would this be less of an issue if the weights are stored in Google Drive though?

windows_app/.DS_Store Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sethaxen
Copy link
Member

Thanks, can you also give the PR a more descriptive name? I'll look through the code more thoroughly shortly.

@unnir unnir requested a review from sethaxen October 17, 2023 08:43
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.

2 participants