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

Add support for templates #13

Merged
merged 45 commits into from
Sep 29, 2023
Merged

Add support for templates #13

merged 45 commits into from
Sep 29, 2023

Conversation

ChristianGeng
Copy link
Member

@ChristianGeng ChristianGeng commented Apr 14, 2023

Closes #5

Introduces audbcard.Datacard that handles the creation of the single datacards with:

dataset = audbcards.Dataset(name, version)
dc = audbcards.Datacard(dataset)
dc.save(name)

It further uses Jinja2 to provide RST templates for the single datacard pages and the overview page listing all datasets.

@ChristianGeng ChristianGeng requested a review from hagenw April 18, 2023 14:06
@ChristianGeng ChristianGeng marked this pull request as ready for review April 25, 2023 11:34
Copy link
Member

@hagenw hagenw left a comment

Choose a reason for hiding this comment

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

Cool, I wasn't aware that you can also easily write RST templates (instead of HTML) using jinja2.

I started with a round of first comments. Maybe you start rebasing this branch and answering to the first comments.

In the mean time I will continue looking into more detail into some of the solutions, e.g. the additional properties and add further comments. But I think overall the templating looks like a good solution and we can integrate this pull request.

audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/dataset.py Show resolved Hide resolved
audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/templates/datacard.j2 Show resolved Hide resolved
tests/templates/db.rst Outdated Show resolved Hide resolved
@hagenw
Copy link
Member

hagenw commented Sep 22, 2023

The templates under audbcards/core/templates/ need to be included in the Python package as they are the default templates.
To do so, you need to add the following line to setup.py:

package_data = {'audbcards': ['core/templates/*']}

What needs then to be added in addition to the code is how to handle user provided templates (that overwrite the default templates), but I think we should tackle this in a different pull request and just focus to provide fixed default templates in this pull request.

@hagenw
Copy link
Member

hagenw commented Sep 22, 2023

This package currently has no docs/ folder with an example dataset, so it's not that easy to inspect the result.
I used the https://github.com/audeering/datasets project to create an HTML page, by modifying its docs/datacard.py file to contain the following content:

import audb
import audbcards
import audeer


# Configuration -----------------------------------------------------------
REPOSITORIES = [
    audb.Repository(
        name='data-public',
        host='https://audeering.jfrog.io/artifactory',
        backend='artifactory',
    ),
]


# Functions to create data cards -------------------------------------------
def run():

    # Set internal repositories
    audb.config.REPOSITORIES = REPOSITORIES

    print('Get list of available datasets... ', end='', flush=True)
    df = audb.available(only_latest=True)
    df = df.sort_index()
    print('done')

    # Clear existing data cards
    audeer.rmdir('datasets')
    audeer.mkdir('datasets')

    # Iterate datasets and create data card pages
    names = list(df.index)
    versions = list(df['version'])
    datasets = []
    for (name, version) in zip(names, versions):
        print(f'Parse {name}-{version}... ', end='', flush=True)
        dataset = audbcards.Dataset(name, version)
        audbcards.core.dataset.create_datacard_page_from_template(dataset)
        datasets.append(dataset)
        print('done')

    # Create datasets overview page
    audbcards.core.dataset.create_datasets_page_from_template(datasets)

This runs through, but it does not produce the required emodb.rst as dc._render_template() seems not to write that file.

@ChristianGeng
Copy link
Member Author

Cool, I wasn't aware that you can also easily write RST templates (instead of HTML) using jinja2.

I started with a round of first comments. Maybe you start rebasing this branch and answering to the first comments.

In the mean time I will continue looking into more detail into some of the solutions, e.g. the additional properties and add further comments. But I think overall the templating looks like a good solution and we can integrate this pull request.

I pulled, but there were no upstream changes that I needed to rebase.

@hagenw
Copy link
Member

hagenw commented Sep 22, 2023

Cool, I wasn't aware that you can also easily write RST templates (instead of HTML) using jinja2.
I started with a round of first comments. Maybe you start rebasing this branch and answering to the first comments.
In the mean time I will continue looking into more detail into some of the solutions, e.g. the additional properties and add further comments. But I think overall the templating looks like a good solution and we can integrate this pull request.

I pulled, but there were no upstream changes that I needed to rebase.

But why is it then showing this:

image

Co-authored-by: Hagen Wierstorf <[email protected]>
audbcards/core/dataset.py Outdated Show resolved Hide resolved
@ChristianGeng
Copy link
Member Author

I created a pull request into this branch for a few of my suggestions at #19.

Merged. Probably allows to close some of the open comments.

audbcards/core/dataset.py Outdated Show resolved Hide resolved
audbcards/core/dataset.py Outdated Show resolved Hide resolved
@hagenw hagenw changed the title 5 add support for templates Add support for templates Sep 26, 2023
@ChristianGeng
Copy link
Member Author

audbcards.core.dataset.create_datasets_page_from_template(datasets) still does not create a RST file, how do I need my example code above in order to include the datacard in the docs?

This should be solved.

Copy link
Member

@hagenw hagenw left a comment

Choose a reason for hiding this comment

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

Cool, I think we are nearly there. I added a few more comments and created #22 to see at a later stage what should be part of audcards.Dataset and what could go directly to audbcards.Datacard having in mind the first gathers information and the second presents them.

tests/conftest.py Outdated Show resolved Hide resolved
tests/test_data/rendered_templates/default.rst Outdated Show resolved Hide resolved
tests/test_data/rendered_templates/default.rst Outdated Show resolved Hide resolved
tests/test_datacard.py Outdated Show resolved Hide resolved
tests/test_datacard.py Outdated Show resolved Hide resolved
tests/test_datacard.py Outdated Show resolved Hide resolved
Co-authored-by: Hagen Wierstorf <[email protected]>
@ChristianGeng
Copy link
Member Author

The templates under audbcards/core/templates/ need to be included in the Python package as they are the default templates. To do so, you need to add the following line to setup.py:

package_data = {'audbcards': ['core/templates/*']}

What needs then to be added in addition to the code is how to handle user provided templates (that overwrite the default templates), but I think we should tackle this in a different pull request and just focus to provide fixed default templates in this pull request.

setup.py has been modified accordingly. I agree that custom templates should be considered as beyond scope here.

@ChristianGeng
Copy link
Member Author

This package currently has no docs/ folder with an example dataset, so it's not that easy to inspect the result. I used the https://github.com/audeering/datasets project to create an HTML page, by modifying its docs/datacard.py file to contain the following content:

import audb
import audbcards
import audeer


# Configuration -----------------------------------------------------------
REPOSITORIES = [
    audb.Repository(
        name='data-public',
        host='https://audeering.jfrog.io/artifactory',
        backend='artifactory',
    ),
]


# Functions to create data cards -------------------------------------------
def run():

    # Set internal repositories
    audb.config.REPOSITORIES = REPOSITORIES

    print('Get list of available datasets... ', end='', flush=True)
    df = audb.available(only_latest=True)
    df = df.sort_index()
    print('done')

    # Clear existing data cards
    audeer.rmdir('datasets')
    audeer.mkdir('datasets')

    # Iterate datasets and create data card pages
    names = list(df.index)
    versions = list(df['version'])
    datasets = []
    for (name, version) in zip(names, versions):
        print(f'Parse {name}-{version}... ', end='', flush=True)
        dataset = audbcards.Dataset(name, version)
        audbcards.core.dataset.create_datacard_page_from_template(dataset)
        datasets.append(dataset)
        print('done')

    # Create datasets overview page
    audbcards.core.dataset.create_datasets_page_from_template(datasets)

This runs through, but it does not produce the required emodb.rst as dc._render_template() seems not to write that file.

This should be resolved already.

@ChristianGeng
Copy link
Member Author

I pushed a test branch at audeering/datasets#8 that uses this branch of audbcards to create a data card for emodb.

@ChristianGeng Unfortunately, installing from the branch via requirements.txt seems not to work as expected (e.g. for audb and setup.cfg it worked at audeering/audb#306.) I will see if I can fix it, but at least you can check out this code, installk the audbcards manually and then build the docs.

The changes in the requirements.txt were correct. There was a missing __init__.py in the core module. This should also work now.

ChristianGeng and others added 5 commits September 27, 2023 15:21
tests/test_datacard.py Outdated Show resolved Hide resolved
@hagenw
Copy link
Member

hagenw commented Sep 29, 2023

The tests are failing here and passing locally for me.

But looking at tests/conftest.py they should fail locally as well as we define the following there when creating the test dataset:

languages=['eng', 'de'],

This most likely means we are still using some cache folder outside of the test tmp folders. I will have a look into it.

@hagenw
Copy link
Member

hagenw commented Sep 29, 2023

I was able to fix the problem:
we were using the same cache folder for audb and audbcards.Dataset in the tests. I changed that and updated tests/test_data/rendered_templates/default.rst to the actual expected output of two languages and to the expected duration.
We create two file, one with duration 1 s and the other with 301 s and we have:

>>> pd.to_timedelta(302, unit='s')
Timedelta('0 days 00:05:02')

@hagenw
Copy link
Member

hagenw commented Sep 29, 2023

Great, I updated the description and we are ready to merge here.

@hagenw hagenw merged commit 13d57b2 into main Sep 29, 2023
@hagenw hagenw deleted the 5-add-support-for-templates branch September 29, 2023 09:04
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.

Add support for templates
2 participants