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

rocAL - Python and Pybind Changes #9

Closed
wants to merge 3 commits into from

Conversation

swetha097
Copy link
Owner

No description provided.

Copy link
Collaborator

@sampath1117 sampath1117 left a comment

Choose a reason for hiding this comment

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

@swetha097 Please address review comments

Looks like some unnecessary files has been added like below
rocAL/rocAL_pybind/amd/pycache/init.cpython-37.pyc
rocAL/rocAL_pybind/amd/rocal/pycache/init.cpython-37.pyc

Some files has been removed
rocAL/rocAL_pybind/amd/rocal/plugin/tf.py
rocAL/rocAL_pybind/amd/rocal/reductions.py

Please check if this is intended

include_directories(${AMDRPP_INCLUDE_DIRS})
include_directories(../rocAL/include/api/

include_directories(../rocAL/include/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove additional line at L98

third_party_lib/
../../amd_openvx/openvx/include/)

add_subdirectory(third_party_lib/pybind11)

pybind11_add_module(rocal_pybind rocal_pybind.cpp)
target_link_libraries(rocal_pybind PRIVATE ${AMDRPP_LIBRARIES} vx_rpp rocal)
target_link_libraries(rocal_pybind PRIVATE rocal vx_rpp amd_rpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check if this change is intended

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be same as before only right?

message("-- ${White}rocal_pybind -- CMAKE_CXX_FLAGS:${CMAKE_CXX_FLAGS}${ColourReset}")
install(TARGETS rocal_pybind DESTINATION lib)
message("-- ${Green}ROCm Augmentation Library Python Binding - rocal_pybind module added ${ColourReset}")
message("-- ${Green}Radeon Augmentation Library Python Binding - rocal_pybind module added ${ColourReset}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change Radeon to ROCm

"max_width": max_decoded_width,
"max_height": max_decoded_height,
"dec_type": decoder_type}
"max_width": max_decoded_width, # 1024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check if this comment is needed ROCm#1024

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these comments in the 3 lines

@@ -164,8 +164,6 @@ def image_random_crop(*inputs, user_feature_key_map=None, path='', file_root='',
"color_format": output_type,
"num_shards": num_shards,
'is_output': False,
"user_key_for_encoded": user_feature_key_map["image/encoded"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are this lines removed?

- Install AMD ROCm using <https://rocmdocs.amd.com/en/latest/Installation_Guide/Installation-Guide.html>
- Pull the docker image containing the example:
```
sudo docker pull abishekr/mlperf_rocm3.5_tf1.15:v0.1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This example looks like uses a ROCm3.5 docker
Please check if we need to change this ROCm 5.4 docker to avoid issue in future

Also ubuntu version mentioned is 18.04 or 16.04
we need to support 20.04 too

from __future__ import print_function
import random
from amd.rocal.plugin.pytorch import ROCALClassificationIterator

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove additional empty line

num_threads = 1
device_id = 0
random_seed = random.SystemRandom().randint(0, 2**32 - 1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove blank line at L47

local_rank = 0
world_size = 1

image_classification_train_pipeline = Pipeline(batch_size=batch_size, num_threads=num_threads, device_id=device_id, seed=random_seed, rocal_cpu=_rali_cpu, tensor_layout=types.NHWC, tensor_dtype=types.FLOAT16)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename rali_cpu to rocal_cpu

image_classification_train_pipeline = Pipeline(batch_size=batch_size, num_threads=num_threads, device_id=device_id, seed=random_seed, rocal_cpu=_rali_cpu, tensor_layout=types.NHWC, tensor_dtype=types.FLOAT16)

with image_classification_train_pipeline:
jpegs, labels = fn.readers.file(file_root=data_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add space before and after = operator
Applicable to all such instances in this file

third_party_lib/
../../amd_openvx/openvx/include/)

add_subdirectory(third_party_lib/pybind11)

pybind11_add_module(rocal_pybind rocal_pybind.cpp)
target_link_libraries(rocal_pybind PRIVATE ${AMDRPP_LIBRARIES} vx_rpp rocal)
target_link_libraries(rocal_pybind PRIVATE rocal vx_rpp amd_rpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be same as before only right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is included?
The pycache files also should be in PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check if this file needs to be in the PR

"max_width": max_decoded_width,
"max_height": max_decoded_height,
"dec_type": decoder_type}
"max_width": max_decoded_width, # 1024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these comments in the 3 lines

@@ -95,7 +95,7 @@ class Pipeline(object):
_handle = None
_current_pipeline = None

def __init__(self, batch_size=-1, num_threads=-1, device_id=-1, seed=1,
def __init__(self, batch_size=-1, num_threads=-1, device_id=-1, seed=-1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this seed=-1 intentional if not change whatever is in TOT

multiplier=pipe._multiplier, offset=pipe._offset,display=display, device=device, device_id = device_id)


class ROCAL_iterator(ROCALGenericImageIterator):
Copy link
Collaborator

Choose a reason for hiding this comment

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

ROCAL_iterator is not required anymore?

super(ROCAL_iterator, self).__init__(pipe)


def draw_patches(img,idx, bboxes):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Draw_patches not required?

image = cv2.cvtColor(image, cv2.COLOR_RGB2BGR )
image = cv2.UMat(image).get()
cv2.imwrite(str(idx)+"_"+"train"+".png", image)
super(ROCALClassificationIterator, self).__init__(pipe, tensor_layout = pipe._tensor_layout, tensor_dtype = pipe._tensor_dtype,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Display argument has been removed?


class ROCALGenericIterator(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this not required?

@@ -22,68 +22,16 @@
from amd.rocal import readers
from amd.rocal import decoders
from amd.rocal import random
from amd.rocal import noise
from amd.rocal import reductions
# from amd.rocal import noise
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include these files do not delete them and uncomment these lines

@swetha097 swetha097 closed this Apr 19, 2023
@swetha097 swetha097 reopened this Apr 19, 2023
@swetha097
Copy link
Owner Author

Closing and creating another PR

@swetha097 swetha097 closed this Apr 19, 2023
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