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 opencv build configuration (optional) #39

Closed
wants to merge 1 commit into from

Conversation

BBuf
Copy link
Contributor

@BBuf BBuf commented Jun 3, 2022

Buddy-compiler is a very interesting project, I had complie and ran edge_detection example successfuly today.

But when I compile, I found there's no compile commands for OpenCV,so this pr add OpenCV compile commands in README.md.

ps: In the future, maybe we can consider DIP Dialect don't depend on opencv, because this dependency is heavy and compling time is so long.

@zhanghb97
Copy link
Member

@BBuf Good idea! These compile commands really helpful for users to access OpenCV in buddy-mlir.
I think we need to find a unified file to write documents that access third-party frameworks. If we write these domain-specific commands in the top README file, it will get bloated as we increase the number of frameworks we support.
Personally, I think maybe we can choose a file like buddy-mlir/docs/DomainSpecificSupports.md or another proper file.
@meshtag mainly works on the image processing side, and he is working on some naming rules and conventions. Any comments about the unified third-party frameworks support docs?

Copy link
Member

@zhanghb97 zhanghb97 left a comment

Choose a reason for hiding this comment

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

As for the details, I think we can give users different choices:

  1. Install with a package manager (apt, yum, etc.) or from source (your current version). In this way, users don't need to specify the OpenCV configuration when building buddy-mlir. The advantage is that it is very convenient to configure.

  2. Only build from source and don't install. In this way, users need to give the configuration (-DOpenCV_DIR=</PATH/TO/OPENCV/BUILD/>) to their build dir when building buddy-mlir. The advantage is that users can switch between different OpenCV versions at any time without affecting the installed environment.

@zhanghb97
Copy link
Member

ps: In the future, maybe we can consider DIP Dialect don't depend on opencv, because this dependency is heavy and compling time is so long.

@BBuf Yes!

DIP Dialect itself does not depend on OpenCV. The DIP C/C++ interface depends on OpenCV.
Currently, we use OpenCV to help us read/write images to the MemRef container. So OpenCV is an essential part for the DIP interface now. In the future, if we have the encode/decode image support in our own MemRef/Img container, then we can let OpenCV as an optional.

As for the encode/decode image file, is there any lightweight methods, or what works can we refer to implement our own support in MemRef container?

@BBuf
Copy link
Contributor Author

BBuf commented Jun 4, 2022

maybe this is a good reference. https://github.com/msnh2012/Msnhnet/blob/master/src/cv/MsnhCVMat.cpp#L60

@meshtag
Copy link
Member

meshtag commented Jun 4, 2022

Personally, I think maybe we can choose a file like buddy-mlir/docs/DomainSpecificSupports.md or another proper file.
@meshtag mainly works on the image processing side, and he is working on some naming rules and conventions. Any comments about the unified third-party frameworks support docs?

Do you think we should create a requirements.txt file so that users can get all dependencies in one go with required versions?

For dependencies which are needed to be built from source, perhaps we can create a unified bash script so that users can get all of them using a single command.

What is your opinion on above ideas?

@meshtag
Copy link
Member

meshtag commented Jun 4, 2022

ps: In the future, maybe we can consider DIP Dialect don't depend on opencv, because this dependency is heavy and compling time is so long.

I agree with this point.

As for the encode/decode image file, is there any lightweight methods, or what works can we refer to implement our own support in MemRef container?

Maybe we can have a look at how boost::gil does it.

Copy link
Member

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

Do you think we should have support for other OS (for ex. Windows) as well?

@BBuf
Copy link
Contributor Author

BBuf commented Jun 4, 2022

Do you think we should have support for other OS (for ex. Windows) as well?

Personally I think it would be great to support windows and macos, so that it will be more convenient for some windows/macos users.

@BBuf
Copy link
Contributor Author

BBuf commented Jun 4, 2022

Personally, I think maybe we can choose a file like buddy-mlir/docs/DomainSpecificSupports.md or another proper file.
@meshtag mainly works on the image processing side, and he is working on some naming rules and conventions. Any comments about the unified third-party frameworks support docs?

Do you think we should create a requirements.txt file so that users can get all dependencies in one go with required versions?

For dependencies which are needed to be built from source, perhaps we can create a unified bash script so that users can get all of them using a single command.

What is your opinion on above ideas?

Maybe another docker image with llvm and opencv installed can be provided.

@zhanghb97
Copy link
Member

I think it's too expensive to maintain documentation and scripts for various domain-specific frameworks (we should consider different OS and different CPUs, which runs the risk of combo explosion). So I prefer that we give a link to the official build/install documentation of each framework, and provide some supporting descriptions like this PR (without the ubuntu-specific part). And maybe providing a docker file is a good idea. However, there is a concern that we are synchronizing the llvm version every day. If the docker file is provided, the daily testing overhead will increase.

Do you think we should create a requirements.txt file so that users can get all dependencies in one go with required versions?

AFAIK, the requirements.txt can only be used in python projects. Please correct me if it can also manage the C++ libraries.


About this PR

@BBuf I have pushed the documentation for writing framework support (see here).
Please move the content of this PR to the above doc.
Personally, I suggest we give the link to OpenCV build/install doc and some supporting description, instead of the ubuntu-specific details of commands. In this way, we can let users know the right way to build/install, and it also reduces our maintenance costs.


Domain-Specific Container

Ideally, I think we should provide our own default encoder/decoder and be compatible with data structures of other frameworks. Take the image processing as an example:

template <typename T, size_t N> class Img : public MemRef<T, N> {
public:
  Img(StringRef fileName); // Default image decoder.
  Img(cv::Mat image); // OpenCV data stracture.
  ... // other frameworks' data structure, boost gil, libpng, etc.
};

@BBuf
Copy link
Contributor Author

BBuf commented Jun 6, 2022

I think it's too expensive to maintain documentation and scripts for various domain-specific frameworks (we should consider different OS and different CPUs, which runs the risk of combo explosion). So I prefer that we give a link to the official build/install documentation of each framework, and provide some supporting descriptions like this PR (without the ubuntu-specific part). And maybe providing a docker file is a good idea. However, there is a concern that we are synchronizing the llvm version every day. If the docker file is provided, the daily testing overhead will increase.

Do you think we should create a requirements.txt file so that users can get all dependencies in one go with required versions?

AFAIK, the requirements.txt can only be used in python projects. Please correct me if it can also manage the C++ libraries.

About this PR

@BBuf I have pushed the documentation for writing framework support (see here). Please move the content of this PR to the above doc. Personally, I suggest we give the link to OpenCV build/install doc and some supporting description, instead of the ubuntu-specific details of commands. In this way, we can let users know the right way to build/install, and it also reduces our maintenance costs.

Domain-Specific Container

Ideally, I think we should provide our own default encoder/decoder and be compatible with data structures of other frameworks. Take the image processing as an example:

template <typename T, size_t N> class Img : public MemRef<T, N> {
public:
  Img(StringRef fileName); // Default image decoder.
  Img(cv::Mat image); // OpenCV data stracture.
  ... // other frameworks' data structure, boost gil, libpng, etc.
};

I have updated OpenCV build docs of windows and linux in here.

@BBuf
Copy link
Contributor Author

BBuf commented Jun 6, 2022

I think it's too expensive to maintain documentation and scripts for various domain-specific frameworks (we should consider different OS and different CPUs, which runs the risk of combo explosion). So I prefer that we give a link to the official build/install documentation of each framework, and provide some supporting descriptions like this PR (without the ubuntu-specific part). And maybe providing a docker file is a good idea. However, there is a concern that we are synchronizing the llvm version every day. If the docker file is provided, the daily testing overhead will increase.

Do you think we should create a requirements.txt file so that users can get all dependencies in one go with required versions?

AFAIK, the requirements.txt can only be used in python projects. Please correct me if it can also manage the C++ libraries.

About this PR

@BBuf I have pushed the documentation for writing framework support (see here). Please move the content of this PR to the above doc. Personally, I suggest we give the link to OpenCV build/install doc and some supporting description, instead of the ubuntu-specific details of commands. In this way, we can let users know the right way to build/install, and it also reduces our maintenance costs.

Domain-Specific Container

Ideally, I think we should provide our own default encoder/decoder and be compatible with data structures of other frameworks. Take the image processing as an example:

template <typename T, size_t N> class Img : public MemRef<T, N> {
public:
  Img(StringRef fileName); // Default image decoder.
  Img(cv::Mat image); // OpenCV data stracture.
  ... // other frameworks' data structure, boost gil, libpng, etc.
};

When I'm free, I will have a try to implement a encode/decode method that does not depend on opencv in Img Class.

@zhanghb97
Copy link
Member

When I'm free, I will have a try to implement a encode/decode method that does not depend on opencv in Img Class.

Nice! Feel free to do this, I will leave this part to you.

@meshtag
Copy link
Member

meshtag commented Jun 6, 2022

AFAIK, the requirements.txt can only be used in python projects. Please correct me if it can also manage the C++ libraries.

I meant to use requirements.txt file only for installing dependencies. For ex. we can install a specific version of OpenCV specified in requirements.txt using pip install -r requirements.txt. Similarly, perhaps we can also specify dependencies for extended set of examples mentioned in this PR (the PR title is a bit misleading I think) using requirements.txt.

@meshtag
Copy link
Member

meshtag commented Jun 8, 2022

Personally, I suggest we give the link to OpenCV build/install doc and some supporting description, instead of the ubuntu-specific details of commands. In this way, we can let users know the right way to build/install, and it also reduces our maintenance costs.

Does this PR require a review? or can we safely close it?
(/cc @zhanghb97 @BBuf )

@BBuf
Copy link
Contributor Author

BBuf commented Jun 8, 2022

can we safely close it?
(/cc @zhanghb97 @BBuf )

Yes, we can close it!

@BBuf BBuf closed this Jun 8, 2022
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.

3 participants