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

Blosc: Warn Unknown Parameters #2715

Merged
merged 1 commit into from
May 29, 2021

Conversation

ax3l
Copy link
Contributor

@ax3l ax3l commented May 25, 2021

Warn the user if unknown parameters are passed to the blosc compressor. Otherwise, typos get quickly unnoticed.

cc @psychocoderHPC @franzpoeschel

Warn the user if unknown parameters are passed to the blosc
compressor. Otherwise, typos get quickly unnoticed.
@ax3l ax3l force-pushed the topic-warnBloscUnknownParam branch from 5c0de15 to 806a553 Compare May 25, 2021 01:07
@eisenhauer
Copy link
Member

eisenhauer commented May 25, 2021

Unfortunately, parameters don't work like that. Blosc gets all the parameters passed to the engine. It can pull off the ones that it understands, but it can't assume that the ones that it doesn't understand are invalid. They may be typos, or they may just be meant for another ADIOS component. To properly detect invalid parameters, we have to have a global registry or something like that. We've talked about it, but not done it. We don't seem to have an issue on it though. May I suggest closing this PR and opening an issue instead?

@psychocoderHPC
Copy link
Contributor

Unfortunately, parameters don't work like that. Blosc gets all the parameters passed to the engine. It can pull off the ones that it understands, but it can't assume that the ones that it doesn't understand are invalid. They may be typos, or they may just be meant for another ADIOS component. To properly detect invalid parameters, we have to have a global registry or something like that. We've talked about it, but not done it. We don't seem to have an issue on it though. May I suggest closing this PR and opening an issue instead?

If I do not miss something the parameters passed are hierarchically organized and belong to a compressor. Invalid parameters should not be part of another compressor. IMO throwing a warning for an unknown compressor should be fine.

//...
    auto blosc = adios.DefineOperator( "compression", "blosc" );
    adios2::Params bloscParams{ { "compressor", "zstd" /*"blosclz"*/ },
                                { "compression_level", "7" }, { "threshold","200" }};

    std::vector< datatype > v( extent );
    std::iota( v.begin(), v.end(), 0 );
    {
        // write
        adios2::Engine engine = IO.Open( "blosctest.bp", adios2::Mode::Write );
        engine.BeginStep();
        auto othervariable = IO.DefineVariable< datatype >(
            "iota", { extent }, { 0 }, { extent } );
        // the parameters should only be visible for blosc
        othervariable.AddOperation( blosc, bloscParams );
        engine.Put( othervariable, v.data() );
        engine.EndStep();
        engine.Close();
    }
//...

@eisenhauer
Copy link
Member

Hmm. Looks like you may be right. Not sure what I was recalling...

Copy link
Member

@JasonRuonanWang JasonRuonanWang left a comment

Choose a reason for hiding this comment

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

Okay I had a check in the compression operator code. The compressor parameters are generated when user applications add operations to a variable. This is separated from the engine parameters or any other parameters. So in principle this vector should not include anything other than the designed parameters for the specific compressor. So it should be fine to give some notifications when an invalid parameter is caught.

The only thing we may need to think later is whether we should output it to std::cerr or throw an exception. Also we need to add this for other compressors as well.

@JasonRuonanWang JasonRuonanWang merged commit f54ce23 into ornladios:master May 29, 2021
@ax3l ax3l deleted the topic-warnBloscUnknownParam branch June 4, 2021 17:26
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.

4 participants