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

progress outputs #1

Closed
jamesb93 opened this issue Jun 12, 2020 · 36 comments · Fixed by #7
Closed

progress outputs #1

jamesb93 opened this issue Jun 12, 2020 · 36 comments · Fixed by #7
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@jamesb93
Copy link
Member

Hello FluCoMites,

It would be nice to have progress outputs from stdout perhaps issued with a verbose flag or something ala Max environment objects. I took a look at implementing myself - alas I lack the C++ skills to really get my head around it but perhaps putting it on your radar might make it find its way into a future release. With some guidance + clarity on how to implement it could be something I take on myself.

@weefuzzy weefuzzy added the enhancement New feature or request label Jun 12, 2020
@weefuzzy
Copy link
Member

Thanks @jamesb93. This is on the radar, but not as a matter of huge urgency to us. There would be two components to making this change:

  1. Make the CLI wrapper use the threaded clients (which is where the progress count will come from)
  2. Implementing the required stdout niceness. For this I was undecied about whether going to look for a nice external library to take care of this and argument handling etc., or to cobble something together and avoid the extra dependency.

Adding the threading isn't totally horrible, but it's not totally obvious either. First step would be to change the template argument in each of the .cpp files to <NRTThreaded[whatever]>, which then gives you the wrapper that runs the separate thread. Then you need something in main to to the polling. One can sort of see how this works in the wrappers for the other hosts, except that (of course) its buried amongst all the other things.

@jamesb93
Copy link
Member Author

Great, that might be doable for myself! I appreciate the guidance. IMO a good stdout would just be the percentage without any other bells and whistles. Maybe a "\r" to clear the screen but then it can be rude to clear peoples consoles without asking, although lots of programs do this (I can think of progress bars in homebrew for example).

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

I had a go at this today, but I'll admit I'm way out of my depth tracking down errors to progress. It seems that the part of the wrapper which takes in the argc and argv is expecting that, so when the wrapper is changed to NRTThreaded it whinges because that doesn't have a run member function. I assume there is just another way of passing arguments to these processes or orchestrating run.

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

Are you doing something like

#include <FluidCLIWrapper.hpp>
#include <clients/rt/LoudnessClient.hpp>

int main(int argc, const char* argv[])
{
  using namespace fluid::client;
  return CLIWrapper<NRTThreadedLoudnessClient>::run(argc, argv);
}

? That should work, at least to the extent of giving you error messages about something other than run (which will be very long and template-y)

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

This is what I get back:

In file included from /Users/james/dev/flucoma/flucoma-cli/src/fluid-ampgate/fluid-ampgate.cpp:12:
/Users/james/dev/flucoma/flucoma-cli/include/FluidCLIWrapper.hpp:393:42: error: too many arguments to function call, expected 0, have 1
    auto         result = client.process(context);
                          ~~~~~~~~~~~~~~ ^~~~~~~
/Users/james/dev/flucoma/flucoma-cli/src/fluid-ampgate/fluid-ampgate.cpp:18:48: note: in instantiation of member function
      'fluid::client::CLIWrapper<NRTThreadedAmpGateClient>::run' requested here
  return CLIWrapper<NRTThreadedAmpGateClient>::run(argc, argv);
                                               ^
/Users/james/dev/flucoma/flucoma-cli/build/_deps/flucoma-core-src/include/clients/rt/../common/FluidNRTClientWrapper.hpp:522:3: note: 'process' declared here
  Result process()
  ^
1 error generated.
make[2]: *** [src/fluid-ampgate/CMakeFiles/fluid-ampgate.dir/fluid-ampgate.cpp.o] Error 1
make[1]: *** [src/fluid-ampgate/CMakeFiles/fluid-ampgate.dir/all] Error 2
make: *** [all] Error 2

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

That could be worse. Just don't pass the instance of FluidContext into process() on line 393 of include/FluidCLIWrapper.hpp

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

Okay I removed passing FluidContext. I assume that the next part is doing some error checking after the work has been complete, and writing when the return is good. I guess at this point you are dealing with pinging the result to see if its done? Are there any good instances of this I can look at in other wrappers?

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

The Result is still useful. The corresponding bit of the PD wrapper might shed some light: https://github.com/flucoma/flucoma-pd/blob/e4d8349c4fa39f08bbee0521585fc0fc19ff443c/include/FluidPDWrapper.hpp#L255

Then for the pinging https://github.com/flucoma/flucoma-pd/blob/e4d8349c4fa39f08bbee0521585fc0fc19ff443c/include/FluidPDWrapper.hpp#L277

However, it'll be slightly different here because there's not a scheduler. So you'll want a loop after you call process that sleeps for a bit (possibly using std::this_thread::sleep_for), then checks the progress. When it's done, break out of the loop and the stuff from line 395 should be the same, I think.

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

Maybe something like

Result result;
client.process();

while(true) 
{
    ProcessState state = client.checkProgress(res);
    if (state == ProcessState::kDone ||
        state == ProcessState::kDoneStillProcessing)
    {
      if (x->checkResult(result)) break; 
    }

    if (state != ProcessState::kDone)
    {
     std::cout << client.progress(); 
     std::this_thread::sleep_for(20ms); 
     continue; 
    }
}

    if (!result.ok())
    {
      // Output error

      std::cerr << result.message() << "\n";
    }
    else
    {
      // Write files

      bool allowCSV = true;
      params.template forEachParamType<BufferT, WriteFiles>(allowCSV);
    }

    return result.ok() ? 0 : -1;

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

I don't remember offhand why we need allowCSV as a thing, but it might have been when Alex was still waiting for me to implement the CSV generation elsewhere

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

I imagine you'll at the very least need to add #include <thread> to the file

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

Okay getting there. It doesn't know what x is but I'm looking at the pd wrapper now to decipher.

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

Okay yep, firmly out my depth here. checkResult seems to be a member function of the other wrappers which is why its passed that instance of the class in pd? So passing this->checkResult fails. Perhaps you can steer me again.

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020 via email

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

Excellent! Let me try this.

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

I should remember to end my lines:

./fluid-nmf -source "/Users/james/Sync/1_PHD/8_Finished Pieces/Stitch-Strata/concat/phrases/phrases6/c29.wav" -resynth foo.wav
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

It looks like something is happening when progress is sent to stdout, but its not what I would expect. I'll push my code to a branch of my fork.

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

💯 💯 💯
That's a lot of zeros. Writing a number to the console every 20ms probably isn't desireable. If you make a variable to hold the progress outside the loop, you can to print only if it's updated by so much:

double progress = 0.0; 
while(true) 
{
//...
    if(client->progress() - progress >= 1) std::cout << client.progress(); 
   progress = client.progress; 
//...

Or, if you'd rather have a more even pace, only update every so many iterations (e.g. 25 for half-secondly updates). Or check less often.

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

Yes these are nicer ways of managing the number. I'll implement them my side, check then put a PR up?

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

Suggest, if you have the will, you live alongside it for a while and confirm it does what you want, especially in a ReaCoMa context. I'm not sure when we're next going to release off this branch, so there's no rush.

@weefuzzy weefuzzy self-assigned this Aug 7, 2020
@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

That said, if you want some feedback on anything, stick a PR up by all means. We can merge whenever.

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 7, 2020

Suggest, if you have the will, you live alongside it for a while and confirm it does what you want

I shall do this. I'd like to point people who use ReaCoMa to the official binaries rather than some bootleg stuff I've built so I'll maintain a branch with progress updates until something like this is integrated in the future.

That said, if you want some feedback on anything, stick a PR up by all means. We can merge whenever.

Sounds good!

@weefuzzy
Copy link
Member

weefuzzy commented Aug 7, 2020

Well, I guess we should be going 1.0.0 before too long, so we can certainly make sure it's in that

@weefuzzy weefuzzy added this to the 1.0.0 milestone Aug 7, 2020
@jamesb93
Copy link
Member Author

jamesb93 commented Aug 8, 2020

So this is where the code is out. I am able to ping the progress of the process but it doesnt actually seem to ever advance I can confirm that with RC1 what I am processing is valid, it complees in 17 seconds. Waiting for 10 minutes and nothing happens with my code. Is there something that is fundamentally missing here? Like a go button or what not.

https://github.com/jamesb93/flucoma-cli/blob/176c2c5293a9815ae944d47b6279b7ba37aa6463/include/FluidCLIWrapper.hpp#L402

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 8, 2020

Ahh perhaps I've found it. Taking inspiration from the pd wrapper, If not done I believe I need to call client.progress()?

https://github.com/flucoma/flucoma-pd/blob/e4d8349c4fa39f08bbee0521585fc0fc19ff443c/include/FluidPDWrapper.hpp#L292

@jamesb93
Copy link
Member Author

jamesb93 commented Aug 8, 2020

Okay nope, nothing there. I'm also periodically getting process and progress confused mentally 👎

@weefuzzy
Copy link
Member

I think you need to enable the threading with client.setSynchronous(false); before calling process()

@jamesb93
Copy link
Member Author

No dice. It just prints 0's. I also tried client.setQueueEnabled(false) after, although I suspect that is more about the queue interface in Max.

@weefuzzy
Copy link
Member

weefuzzy commented Aug 10, 2020 via email

@jamesb93
Copy link
Member Author

No worries. Thanks for all your help so far

@weefuzzy
Copy link
Member

weefuzzy commented Aug 23, 2020

There was one crucial missing element, in that we needed to enqueue the job before calling process. We also should have been checking the return value of process at this would have told us what was going wrong straight off.

Try this:

    // Create client after all parameters are set
    ClientType   client(params);
    Result result;

    client.enqueue(params);
    result = client.process();
    
    double progress = 0.0; // Variable to store progress

    while(result.ok())
    {
        ProcessState state = client.checkProgress(result);
      
        if (state == ProcessState::kDone || state == ProcessState::kDoneStillProcessing) {
          std::cout << '\n';
          break;
        }
        if (state != ProcessState::kDone) {
          double newProgress = client.progress();
          if (newProgress - progress >=0.01)
          {
            std::cout << newProgress << '\r' << std::flush;
            
            progress = newProgress;
          }
          using namespace std::chrono_literals;
          std::this_thread::sleep_for(20ms); 
          continue; 
        }
    }

    if (!result.ok())
    {
      // Output error
      std::cerr << result.message() << "\n";
    }
    else
    {
      // Write files
      bool allowCSV = true;
      params.template forEachParamType<BufferT, WriteFiles>(allowCSV);
    }

    return result.ok() ? 0 : -1;
  }

The significant changes are:

  1. We actually queue the job
  2. The while loop only runs if there wasn't an immediate problem with the invocation
  3. I've used \r and a flush for the progress so that it looks nicer in terminal. YMMV

I moved the using namespace to next to the usage site as well, to make it less confusing. Diff attached.
progress-out-diff.txt

@jamesb93
Copy link
Member Author

Uh oh! Is this an easy one you've come across? I did brew update && brew upgrade not too long ago so perhaps it updated one of the underlying libs required for compilation?

-- The CXX compiler identification is AppleClang 11.0.0.11000033
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ - broken
CMake Error at /usr/local/Cellar/cmake/3.18.2/share/cmake/Modules/CMakeTestCXXCompiler.cmake:59 (message):
The C++ compiler

"/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++"

is not able to compile a simple test program.

It fails with the following output:

Change Dir: /Users/james/dev/flucoma/flucoma-cli/build/CMakeFiles/CMakeTmp

Run Build Command(s):/usr/bin/make cmTC_e034f/fast && /Applications/Xcode.app/Contents/Developer/usr/bin/make  -f CMakeFiles/cmTC_e034f.dir/build.make CMakeFiles/cmTC_e034f.dir/build
Building CXX object CMakeFiles/cmTC_e034f.dir/testCXXCompiler.cxx.o
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++   -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.8 -o CMakeFiles/cmTC_e034f.dir/testCXXCompiler.cxx.o -c /Users/james/dev/flucoma/flucoma-cli/build/CMakeFiles/CMakeTmp/testCXXCompiler.cxx
warning: include path for stdlibc++ headers not found; pass '-stdlib=libc++' on the command line to use the libc++ standard library instead [-Wstdlibcxx-not-found]
1 warning generated.
Linking CXX executable cmTC_e034f
/usr/local/Cellar/cmake/3.18.2/bin/cmake -E cmake_link_script CMakeFiles/cmTC_e034f.dir/link.txt --verbose=1
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.8 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_e034f.dir/testCXXCompiler.cxx.o -o cmTC_e034f
clang: warning: libstdc++ is deprecated; move to libc++ with a minimum deployment target of OS X 10.9 [-Wdeprecated]
ld: library not found for -lstdc++
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [cmTC_e034f] Error 1
make: *** [cmTC_e034f/fast] Error 2

CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
CMakeLists.txt:25 (project)

-- Configuring incomplete, errors occurred!
See also "/Users/james/dev/flucoma/flucoma-cli/build/CMakeFiles/CMakeOutput.log".
See also "/Users/james/dev/flucoma/flucoma-cli/build/CMakeFiles/CMakeError.log".

@weefuzzy
Copy link
Member

Have you upgraded Xcode to 11 recently?

@weefuzzy
Copy link
Member

And is this from a clean build folder?

@jamesb93
Copy link
Member Author

I haven't explicitly updated Xcode but maybe something automatically did it. I'll clean build and see what happens. It's likely it was dirty from before.

@jamesb93
Copy link
Member Author

Okay cleaning out the build folder worked. In this case Green's paradox didn't hold fast and as the codebase got dirtier it didn't run better.

jamesb93 added a commit to jamesb93/flucoma-cli that referenced this issue Aug 26, 2020
This was linked to pull requests Sep 30, 2020
@weefuzzy weefuzzy removed a link to a pull request Sep 30, 2020
@weefuzzy
Copy link
Member

@jamesb93 I've tagged you in #7 hoping that you can give this a test drive before we merge

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 a pull request may close this issue.

2 participants